[BD-46] feat: add Multiselect component #1498
Conversation
|
Thanks for the pull request, @monteri! When this pull request is ready, tag your edX technical lead. |
openedx-webhooks
added
the
blended
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov ReportBase: 91.61% // Head: 91.67% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1498 +/- ##
==========================================
+ Coverage 91.61% 91.67% +0.06%
==========================================
Files 208 210 +2
Lines 3541 3581 +40
Branches 823 829 +6
==========================================
+ Hits 3244 3283 +39
- Misses 282 283 +1
Partials 15 15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/Multiselect/Multiselect.scss
Outdated
| @@ -0,0 +1,126 @@ | |||
| // Put styles related to your component here | |||
src/Multiselect/Multiselect.scss
Outdated
|
|
||
| &.show { | ||
| z-index: $multiselect-floating-label-zindex; | ||
| padding: 0 .5em; |
There was a problem hiding this comment.
nit: should this be rem?
There was a problem hiding this comment.
Yes, correct, mistake
|
|
||
| const ClearIndicator = (props) => ( | ||
| <components.ClearIndicator {...props}> | ||
| <Icon src={Close} /> |
There was a problem hiding this comment.
I'm curious if we should be using an IconButton here instead of a regular Icon to get its focus/hover styles, though I imagine it might mess with the spacing inside of the input. There might also be a11y implications for rendering a clickable icon outside of an element that indicates its an interactive element (i.e., its not currently inside of a button element or an element with a role="button" for example).
There was a problem hiding this comment.
Used IconButton instead of default component. Styled to fit the design. Added some changes according to focus/hover.
|
|
||
| const DropdownIndicator = (props) => ( | ||
| <components.DropdownIndicator {...props}> | ||
| <Icon src={KeyboardArrowDown} /> |
There was a problem hiding this comment.
Similar question regarding IconButton vs Icon
| @@ -0,0 +1,136 @@ | |||
| import React from 'react'; | |||
| import classNames from 'classnames'; | |||
| import { components } from 'react-select'; | |||
There was a problem hiding this comment.
nit: import { components as reactSelectComponents } from 'react-select'; just to be a tad more explicit
| </components.ValueContainer> | ||
| ); | ||
|
|
||
| const Control = ({ |
There was a problem hiding this comment.
[curious] Did you attempt to make use of Form.Control in any way here to avoid having to replicate CSS styles? Might not be possible or overly complex, but just wondering if we explored that avenue 😄
There was a problem hiding this comment.
I didn't try because if I place Form.Control instead of Control it will appear as Form.Control inside another Form.Control and there a lot of change will be needed vs make some style and logic updates. The only problem that took a lot of time was to set up floating label.
| <components.Option className={classNames({ 'is-focus': props.isFocused })} {...props} /> | ||
| ); | ||
|
|
||
| const MultiValueContainer = ({ innerProps, ...props }) => { |
There was a problem hiding this comment.
[curious] Did we try to explore whether we could make use of the existing Chip here? The Chip-like elements in this multiselect implementation are different from the behavior of the Paragon Chip component in that the entire element is clickable to dismiss it, not just the icon.
Related, is there any way to focus with keyboard only navigation on these chips for a11y support?
There was a problem hiding this comment.
Yes, Chip in the Multiselect is different from the Paragon's Chip and actually it was easier to change some styles to fit design vs replacing whole component. In last commit I added a11y support (focus for Chip icon and onKeyPress activates onClick)
src/Multiselect/README.md
Outdated
|
|
||
| return ( | ||
| <Stack direction="vertical"> | ||
| <div className="w-100 p-4"> |
There was a problem hiding this comment.
I don't think the horizontal padding is necessary for the examples without a background as it creates unnecessary whitespace.
monteri
force-pushed
the
zadorozhnii/add-multiselect
branch
from
ff82a82
to
5709f4e
Compare
monteri
force-pushed
the
zadorozhnii/add-multiselect
branch
from
5709f4e
to
0648468
Compare
monteri
force-pushed
the
zadorozhnii/add-multiselect
branch
from
0648468
to
32d0a0b
Compare
Description
component-generatorto place proper quotesDeploy Preview
Multiselect
Merge Checklist
exampleapp?wittjeffandadamstankiewiczas reviewers on this PR.Post-merge Checklist