NotificationsYou must be signed in to change notification settings
Fork920
Star10.1k

chore(site): refactor filter component to be more extendable#13688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

BrunoQuaresma merged 16 commits intomainfrombq/merge-search-and-search-field

Jul 2, 2024

Merged

chore(site): refactor filter component to be more extendable#13688

BrunoQuaresma merged 16 commits intomainfrombq/merge-search-and-search-field

Jul 2, 2024

Conversation

Copy link

Collaborator

BrunoQuaresma commentedJun 27, 2024

Related to#13035

BrunoQuaresma added8 commits

June 25, 2024 19:29

Replace search by menu search

d222f72

Add select menu component

050f2c9

Add select filter menu

d4c183b

Replace filter menu by SelectFilter

0706238

Fix search icons

60aa016

Fix filter buttons size

08b0a82

Improve accessibility and searching

c7e15ec

Fix width of popover and move search to has its own component

2e5880f

BrunoQuaresma requested review fromaslilac andParkreiner

June 27, 2024 13:57

BrunoQuaresma self-assigned this

Jun 27, 2024

Copy link

CollaboratorAuthor

BrunoQuaresma commentedJun 27, 2024

@aslilac @Parkreiner I would appreciate a QA on this too 🙏

Copy link

Member

Parkreiner commentedJun 27, 2024

@BrunoQuaresma I'll definitely start the review later today, but just because this is such a core piece of the app, I'm really going to want to take my time with testing it out locally

Might not be able to start that until tomorrow morning, but I'll try to get review and testing wrapped up by EOD tomorrow

aslilac reviewed

Jun 27, 2024

View reviewed changes

site/src/components/Avatar/Avatar.tsx OutdatedShow resolvedHide resolved

site/src/components/Filter/SelectFilter.tsx OutdatedShow resolvedHide resolved

site/src/components/SelectMenu/SelectMenu.tsx OutdatedShow resolvedHide resolved

site/src/components/SelectMenu/SelectMenu.tsxShow resolvedHide resolved

Parkreiner reviewed

Jun 27, 2024

View reviewed changes

Copy link

Member

Parkreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good so far!
I'm going to try running this tomorrow morning

site/src/components/Avatar/Avatar.tsx OutdatedShow resolvedHide resolved

site/src/components/SelectMenu/SelectMenu.tsx OutdatedShow resolvedHide resolved

site/src/components/Filter/menu.tsShow resolvedHide resolved

site/src/components/Filter/SelectFilter.tsx Outdated

Comment on lines 34 to 35

		// SelectFilterSearch element
		search?: ReactNode;

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could this prop be renamed to make it more clear that it's meant to be used withSelectFilterSearch? It doesn't feel obvious right now, especially with the comment being a private comment

Copy link

CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

When usingSelectFilterSearch I feel I have to pass the component and not a React Node 🤔 MaybeselectFilterSearch?

site/src/components/Filter/SelectFilter.tsx Outdated

Comment on lines 56 to 57

		{selectedOption?.label ?? placeholder}
		<span css={{ ...visuallyHidden }}>{label}</span>

Copy link

Member

ParkreinerJun 27, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you explain what you were going for with this? From how I'm reading it, if theselectedOption has a label, then both it and the visually-hidden label in the span will be exposed through the button as an accessible name value. So in that case, a screen-reader user might get double/redundant labels

If it's okay for the main visual label text to be hidden from screen readers, this could be rewritten as

<spanaria-hidden>{selectedOption?.label??placeholder}</span>

This hides it from screen readers, but keeps the content on the page. Normally this is a really bad idea, but as long as we have the backup label guaranteed to be defined, I think it's okay

Edit:aria-hidden, nothidden

Copy link

CollaboratorAuthor

BrunoQuaresmaJun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I didn't notice it 🤦 My idea is, since this is a select component, I think having a label like "Select a user" is better than having the selected value as the label "User 2" for example.

Copy link

CollaboratorAuthor

BrunoQuaresmaJun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I usedaria-label in the button directly.

site/src/components/Filter/UserFilter.tsx OutdatedShow resolvedHide resolved

Update site/src/components/Filter/UserFilter.tsx

3fae8d4

Co-authored-by: Michael Smith <throwawayclover@gmail.com>

Copy link

Member

Parkreiner commentedJun 28, 2024

@BrunoQuaresma Ran through and tested everything. Everything seems to work really well. Only noticed one minor quirk when using the menus via keyboard

When the menu opens up, it would be nice if the first element in the menu gets autofocus (for the list-only dropdowns, that would be the first item, but for the search dropdowns, that would be the search box). Without the auto-focus, trying to hit the arrow keys just makes the menu scroll instead of interacting with the elements

Screen.Recording.2024-06-28.at.10.36.47.AM.mov

Even for non-keyboard users, auto-focusing the search field feels like a pretty good UI improvement, because it means one less click that they need to do

Parkreiner approved these changes

Jun 28, 2024

View reviewed changes

Copy link

Member

Parkreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Gonna go ahead and approve this – the core redesign seems really good, and none of the feedback seems big enough to be worth blocking on

BrunoQuaresma added4 commits

June 28, 2024 18:13

Remove important style

06ae934

Apply review comments

0d83bc8

Merge branch 'bq/merge-search-and-search-field' ofhttps://github.com…

59bd176

…/coder/coder into bq/merge-search-and-search-field

Add auto focus to search

928209c

BrunoQuaresma requested a review fromaslilac

June 28, 2024 18:49

Copy link

CollaboratorAuthor

BrunoQuaresma commentedJun 28, 2024

@Parkreiner I would appreciate a second round of review if possible 🙏 . Related to autofocus, it does not work locally, but to make sure I'm going to test it on a preview deployment. Thanks for the heads up.

Fix types

d344d9c

coder deleted a comment fromgithub-actionsbot

Jul 1, 2024

Copy link

github-actionsbot commentedJul 1, 2024

🚀 Deploying PR 13688 ...

Parkreiner reviewed

Jul 2, 2024

View reviewed changes

site/src/components/Filter/SelectFilter.stories.tsx OutdatedShow resolvedHide resolved

Parkreiner approved these changes

Jul 2, 2024

View reviewed changes

Copy link

Member

Parkreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Did a quick look over the new files, and the code looks pretty good to me. I know you're usingsplice now, but it looked like was still using immutable operations, so we're good

Tentatively giving my rubber stamp – not sure if@aslilac wants to kick the tires a bit more?

Copy link

CollaboratorAuthor

BrunoQuaresma commentedJul 2, 2024

Looks like preview deployments are broken so I'm going to merge it and test "auto focus" on dogfood.

aslilac approved these changes

Jul 2, 2024

View reviewed changes

Copy link

Member

aslilac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

looks good! couple small thoughts

site/src/components/SelectMenu/SelectMenu.stories.tsxShow resolvedHide resolved

site/src/pages/WorkspacesPage/filter/menus.tsxShow resolvedHide resolved

BrunoQuaresma added2 commits

July 2, 2024 16:00

Apply PR suggestions

ebcbaa5

Fix closed story

50a6ce4

BrunoQuaresma merged commit9ee53e5 intomain

Jul 2, 2024

29 checks passed

BrunoQuaresma deleted the bq/merge-search-and-search-field branch

July 2, 2024 16:15

github-actionsbot locked and limited conversation to collaborators

Jul 2, 2024

Labels

None yet

3 participants

Movatterモバイル変換

chore(site): refactor filter component to be more extendable#13688

chore(site): refactor filter component to be more extendable#13688

Uh oh!

Conversation

BrunoQuaresma commentedJun 27, 2024

Uh oh!

BrunoQuaresma commentedJun 27, 2024

Uh oh!

Parkreiner commentedJun 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ParkreinerJun 27, 2024

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresmaJun 28, 2024

Choose a reason for hiding this comment

Uh oh!

ParkreinerJun 27, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresmaJun 28, 2024

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresmaJun 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Parkreiner commentedJun 28, 2024

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma commentedJun 28, 2024

Uh oh!

github-actionsbot commentedJul 1, 2024

🚀 Deploying PR 13688 ...

Uh oh!

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

BrunoQuaresma commentedJul 2, 2024

Uh oh!

aslilac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ParkreinerJun 27, 2024•
edited
Loading