-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(react): upgrade downshift dependency to v5 #5373
feat(react): upgrade downshift dependency to v5 #5373
Conversation
Deploy preview for carbon-elements ready! Built with commit 2880d29 |
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.
Left a quick nit for the function annotation, would we want to add a quick test for mapDownshiftProps
?
Deploy preview for carbon-components-react ready! Built with commit 2880d29 https://deploy-preview-5373--carbon-components-react.netlify.app |
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.
Noticing a couple of issues:
Combobox
- Combobox
light
prop does not seem to work
Dropdown
- [ ] [Inline variant] option bottom border is a bit short on the right side
WIll be fixed via #5389
MultiSelect
- [ ] [Inline variant] option bottom border is a bit short on the right side
Will be fixed via #5389
Few comments but otherwise looks great, thanks for taking this on! Has been in the backlog for quite some time... #2628 😂 |
@tw15egan great catch on the light prop not applying 👏 It looks like in our styling we were grabbing As for the other two bugs you mentioned -- it seems like those may have been introduced in an earlier commit? I'm seeing those on the netlify preview |
@tw15egan is it one pixel too much padding on the left side? |
@dakahn it looks like it's coming from this line. I think we can fix this by keeping that line (needed to fix focus issue on normal Dropdowns) but adding in an inline-specific override like .#{$prefix}--list-box--inline .#{$prefix}--list-box__field {
height: 100%;
} Last style issue I'm seeing, everything else is looking great! |
…on into upgrade-downshift-dependency
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.
Awesome work 👍 ✅ 🏄♂️ 🎉
Closes #2628
Closes #5335
Closes #2539
Closes #4373
Closes #5211
Closes #3419
Closes #3416
Upgrade downshift dependency from v1 to v5 so we could take advantage of improved accessibility and a
useSelect
hook allowing Windows screen readers to enter forms mode appropriately.Changed:
button
button-reset
combobox
totype='text'
span
to alabel
useSelect
hookListBox.Field
tobutton
elementReact.ForwardRef
innerRef
hookReact.ForwardRef
innerRef
role="combobox"
aria-haspopup="listbox"
aria-expanded
andaria-haspopup
getButtonProps
togetToggleButtonProps
innerRef
useSelect
hooknoop
value fromundefined
to{}
stateChangeTypes
to new change types for keyboard/mouse interactionsListBox.Field
tobutton
elementuseCallback
useState
hooksuseSelection
functioncreatePropAdapter
for handling deprecated propsTesting:
(if you find errors cross check for existing issues in our backlog)
non-blocking known issues: