-
Notifications
You must be signed in to change notification settings - Fork 67
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BD-46] feat: add Multiselect component #1498
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @monteri! When this pull request is ready, tag your edX technical lead. |
✅ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unneeded :)
src/Multiselect/Multiselect.scss
Outdated
|
||
&.show { | ||
z-index: $multiselect-floating-label-zindex; | ||
padding: 0 .5em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should this be rem
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct, mistake
|
||
const ClearIndicator = (props) => ( | ||
<components.ClearIndicator {...props}> | ||
<Icon src={Close} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar question regarding IconButton
vs Icon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here
@@ -0,0 +1,136 @@ | |||
import React from 'react'; | |||
import classNames from 'classnames'; | |||
import { components } from 'react-select'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the horizontal padding is necessary for the examples without a background as it creates unnecessary whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to py-4
ff82a82
to
5709f4e
Compare
5709f4e
to
0648468
Compare
0648468
to
32d0a0b
Compare
Description
component-generator
to place proper quotesDeploy Preview
Multiselect
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist