Multiselect rewrite #743
Multiselect rewrite #743
Conversation
Will need to properly style scrollbars on windows later
|
This pull request has been linked to Shortcut Story #12060: Center align all buttons. |
There was a problem hiding this comment.
this looks so good! tested with browserstack and looks good to me. i just have some comments that aren't going to affect functionality regardless of what you do with my thoughts! going to approve so that this merge isn't blocking the push to demo site and further VQA
| selectionsCount={filterContext.filterSelections.ownernames.length} | ||
| className="ownernames-accordion" | ||
| i18n={i18n} |
There was a problem hiding this comment.
could the i18n just be imported in the FilterAccordion file rather than being passed as a prop? same with MultiSelect component below
client/src/styles/_alert.scss
Outdated
| align-self: flex-start; | ||
| // TODO: target area of button is still less than 44x44 px for a11y |
There was a problem hiding this comment.
is there a ticket for this? if so i'd err on the side of not leaving in TODO comments
There was a problem hiding this comment.
yeah good point - added a shortcut card to the design system collection and will remove this and others.
| } | ||
| return value.toString().indexOf(search) > -1; | ||
| } | ||
| const ariaLiveGuidanceGeneral = `Type to refine list of options. Use Up and Down to choose options, |
There was a problem hiding this comment.
didn't know this was a thing!! 😮
|
|
||
| // Passing custom prop to make accessible to child compnents via selectProps | ||
| const removeValue = useCallback( | ||
| // @ts-ignore (TODO: says property 'value' doesn't exist on type Option) |
There was a problem hiding this comment.
hm, can we extend the type Option to include the property value? kind of confused about this error
There was a problem hiding this comment.
Yeah I don't understand it either, because the Option type has value, and it knows it has this type, but for some reason it's doesn't know what the properties are.
At the top of this files it's defined as:
export type Option = {
value: string;
label: string;
[key: string]: string;
};
| Label: components.MultiValueLabel, | ||
| Remove: CustomMultiValueRemove, | ||
| }} | ||
| // TODO: Can't know when it's "focused" via arrow key navigation, it's not using ::focus, |
There was a problem hiding this comment.
thoughts on making github issues for the things that aren't breaking errors but would be nice to fix? unsure if that's a good use of our time but i dont know if these non-breaking errors are being kept track of anywhere. they don't feel urgent enough to be addressed right now in VQA, but I think we do need a visible log of them
There was a problem hiding this comment.
oh yeah good point, made this (#747) and I'll remove the comment.
This reverts commit b85c223.
Previously (#743) I tried to remove i18n as a prop from some of the filters components and instead just import it in those files. However, I misunderstood exactly how this is supposed to work and since we didn't have all the translations in I didn't notice that it broke at first. This PR fixes those issues using the proper ways to pass around the correct i18n prop without having to manually add it each time, following the examples elsewhere in wow. This makes use of <I18n> {({i18n}) => ()} </I18n> pattern for the multiselect and minmaxselect components, and the withI18nProps & withI18n() pattern for the Filters and Table components.
The original multiselect library I started with for filters has turned out to be not that great - it was not originally written in typescript, it's difficult to customize, and it's not accessible for screen readers. So after struggling to add screen-reader accessibility to the existing component, I've just decided to use the much better
react-selectlibrary and rewrite our custom multiselect filter.react-selectis written in typescript, has great accessibility features (nicely handles keyboard navigation for search-bar/option-list/selected-list, and has great default screen reader setting plus customization options), and it has a nice system for customizing by overwriting and recomposing its subcomponents. (The entireMultiSelect.tsxfile is new, so you can just review that file alone and ignore the diff)Since I then needed to update a lot of the filter styles for the new component, I ended up doing a much needed refactor of all the PortfolioFilters styles. I pulled out the minmax-select and multi-select styles into their one contained component styles, and also tried to simplify the organization of desktop/mobile styles to keep things close together (rather than the mostly separate mobile-specific styles nested in it's own section. I also tried to pull other changes out to component styles (Alerts, Buttons), and address some other issues at the same time.
While I was separating out the minmax-select component styles I decided to pull the component into it's own file as well (the contents haven't changed).
This all addresses a few outstanding tickets: