chore(site): refactor filter component to be more extendable #13688
Conversation
|
@aslilac @Parkreiner I would appreciate a QA on this too 🙏 |
|
@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 |
| // SelectFilterSearch element | ||
| search?: ReactNode; |
There was a problem hiding this comment.
Could this prop be renamed to make it more clear that it's meant to be used with SelectFilterSearch? It doesn't feel obvious right now, especially with the comment being a private comment
There was a problem hiding this comment.
When using SelectFilterSearch I feel I have to pass the component and not a React Node 🤔 Maybe selectFilterSearch?
| {selectedOption?.label ?? placeholder} | ||
| <span css={{ ...visuallyHidden }}>{label}</span> |
There was a problem hiding this comment.
Can you explain what you were going for with this? From how I'm reading it, if the selectedOption 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
<span aria-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, not hidden
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I used aria-label in the button directly.
Co-authored-by: Michael Smith <throwawayclover@gmail.com>
|
@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.movEven 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 |
…/coder/coder into bq/merge-search-and-search-field
|
@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. |
🚀 Deploying PR 13688 ... |
There was a problem hiding this comment.
Did a quick look over the new files, and the code looks pretty good to me. I know you're using splice 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?
|
Looks like preview deployments are broken so I'm going to merge it and test "auto focus" on dogfood. |
Related to #13035