Skip to content
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

Merged
merged 118 commits into from
May 7, 2020
Merged

feat(react): upgrade downshift dependency to v5 #5373

merged 118 commits into from
May 7, 2020

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Feb 17, 2020

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:

  • packages/components/src/components/button/_mixins.scss
    • added disabled styling
  • packages/components/src/components/dropdown/_dropdown.scss
    • changed targeted triggering element to a button
    • added button-reset
    • remove redundant/unneeded aria
  • packages/components/src/components/list-box/_list-box.scss
    • changed targeted role from combobox to type='text'
  • packages/react/src/components/Dropdown/Dropdown-test.js
    • changed label lookup from a span to a label
  • packages/react/src/components/Dropdown/Dropdown.js
    • refactored for React Hooks
    • added Downshift's useSelect hook
    • removed redundant/unneeded aria
    • changed ListBox.Field to button element
  • packages/react/src/components/ListBox/ListBox.js
    • added React.ForwardRef
    • remove innerRef hook
    • remove redundant/unneeded aria
  • packages/react/src/components/ListBox/ListBoxMenu.js
    • added React.ForwardRef
  • packages/react/src/components/ListBox/tests/ListBox-test.js
    • remove tests for innerRef
  • packages/react/src/components/ListBox/test-helpers.js
    • add lookup for role="combobox"
    • add lookup for aria-haspopup="listbox"
    • remove helpers for aria-expanded and aria-haspopup
  • packages/react/src/components/MultiSelect/FilterableMultiSelect.js
    • change deprecated getButtonProps to getToggleButtonProps
    • remove innerRef
  • packages/react/src/components/MultiSelect/MultiSelect.js
    • refactored for React Hooks
    • added Downshift's useSelect hook
    • removed redundant/unneeded aria
    • changed noop value from undefined to {}
    • refactor deprecated stateChangeTypes to new change types for keyboard/mouse interactions
    • changed ListBox.Field to button element
  • packages/react/src/internal/Selection.js
    • add useCallback useState hooks
    • add useSelection function
  • packages/react/src/prop-types/childrenOfType.js
    • add early return if there is not child element
  • packages/react/src/tools/createPropAdapter.js
    • add createPropAdapter for handling deprecated props

Testing:

  • Check for DAP errors on open and closed states for all dropdown components and their variants
    • MultiSelect
    • FilterableMultiselect
    • Dropdown
    • Comobox
      (if you find errors cross check for existing issues in our backlog)
  • Confirm issues are closed by this PR for:
    • VoiceOver for macOS on Safari latest
    • JAWS 2019^ on Windows 10 for Firefox and Chrome latest
    • NVDA on Windows 10 for Firefox and Chrome latest

non-blocking known issues:

  • "initial DOM focus should be on its textbox when a combobox receives focus" DAP error on Combobox is unrelated to these changes

@dakahn dakahn requested a review from a team as a code owner February 17, 2020 23:11
@ghost ghost requested review from abbeyhrt and emyarod February 17, 2020 23:11
@dakahn dakahn changed the title Upgrade downshift dependency [WIP]feat(react): upgrade downshift dependency to v5 Feb 17, 2020
@netlify
Copy link

netlify bot commented Feb 17, 2020

Deploy preview for carbon-elements ready!

Built with commit 2880d29

https://deploy-preview-5373--carbon-elements.netlify.app

Copy link
Contributor

@joshblack joshblack left a 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?

packages/react/src/tools/createPropAdapter.js Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Feb 17, 2020

Deploy preview for carbon-components-react ready!

Built with commit 2880d29

https://deploy-preview-5373--carbon-components-react.netlify.app

Copy link
Member

@tw15egan tw15egan left a 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

@tw15egan
Copy link
Member

Few comments but otherwise looks great, thanks for taking this on! Has been in the backlog for quite some time... #2628 😂

@dakahn
Copy link
Contributor Author

dakahn commented Feb 18, 2020

@tw15egan great catch on the light prop not applying 👏 It looks like in our styling we were grabbing role="combobox" which isn't there anymore after the v5 update and should be updated to type="text". I'll push up a fix.

Screen Shot 2020-02-18 at 11 52 39 AM

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 :hurtrealbad:

@tw15egan
Copy link
Member

tw15egan commented May 5, 2020

Few things I'm noticing:

Dropdown

  • Top menu orientation doesn't seem to be doing anything

MultiSelect

  • Top menu orientation doesn't seem to be doing anything
  • The inline version seems to be off by 1px somewhere

Screen Shot 2020-05-05 at 9 00 22 AM

(Dropdown seems to be look correct)
Screen Shot 2020-05-05 at 9 00 29 AM

@dakahn
Copy link
Contributor Author

dakahn commented May 5, 2020

@tw15egan is it one pixel too much padding on the left side?

@dakahn dakahn requested a review from joshblack May 6, 2020 15:48
@tw15egan
Copy link
Member

tw15egan commented May 6, 2020

@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!

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work 👍 ✅ 🏄‍♂️ 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants