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(selection): introduce selection related hooks and containers #9

Merged
merged 19 commits into from
Feb 27, 2019

Conversation

ryanseddon
Copy link
Contributor

@ryanseddon ryanseddon commented Jan 25, 2019

Description

This adds a hook and render-prop container for selection. useSelection and <SelectionContainer>.

Detail

This borrows heavily from @Austin94 exploratory work on a roving tab index hook as opposed to our previous strategy in SelectionContainer of using the aria-activedescendant to control a virtual focus this now just shifts actual browser focus which AFAICT is fine to use either or.

Checklist

  • 🌐 Storybook demo is up-to-date (yarn storybook)
  • 💂‍♂️ includes new unit tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

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

This is a great start! Some additional questions:

  • How does this work with RTL? I'm assuming we haven't explicitly solved that yet, but we could provide a simple component that uses the useContext hook rather than relying on the styled-components method in react-components.

packages/selection/src/useControlled.js Outdated Show resolved Hide resolved
packages/selection/src/useField.js Outdated Show resolved Hide resolved
packages/selection/src/useField.js Outdated Show resolved Hide resolved
packages/selection/src/useSelection.js Outdated Show resolved Hide resolved
packages/selection/src/useSelection.js Outdated Show resolved Hide resolved
packages/selection/src/useSelection.js Outdated Show resolved Hide resolved
@austingreendev
Copy link
Contributor

Also, assuming this is just a side-effect of it being WIP, but the focus entrance/exit seems to not align with the selected value.

invalid-focus-order

@ryanseddon
Copy link
Contributor Author

Thanks for the early feedback @Austin94

How does this work with RTL?

Haven't got to that yet still trying to put the basics in place first.

Also, assuming this is just a side-effect of it being WIP, but the focus entrance/exit seems to not align with the selected value.

Yeah same answer as above

… logic so correct tabIndex is toggled based on focus/selection
@ryanseddon
Copy link
Contributor Author

Also, assuming this is just a side-effect of it being WIP, but the focus entrance/exit seems to not align with the selected value.

@Austin94 this is address in 6edbf9c

packages/selection/README.md Outdated Show resolved Hide resolved
packages/selection/package.json Outdated Show resolved Hide resolved
packages/selection/package.json Outdated Show resolved Hide resolved
packages/selection/src/useSelection.js Outdated Show resolved Hide resolved
packages/selection/src/useSelection.js Outdated Show resolved Hide resolved
packages/selection/src/useSelection.js Outdated Show resolved Hide resolved
@ryanseddon
Copy link
Contributor Author

ryanseddon commented Feb 26, 2019

@Austin94 re: dd12c5f

Reason for removing Locale component is a don't think we should offer it here for two reasons:

  • In react-components ThemeProvider will pass down rtl for us and we can just pass along isRtl(props) to the hook
  • Outside consumers building upon these hooks will probably have their own context provider to switch RTL

I think this makes rtl less magic and more obvious to people building with this.

@austingreendev
Copy link
Contributor

It think this makes rtl less magic and more obvious to people building with this.

The less magic the better! 🎉

If someone really wanted it they could wrap it with a custom useLocaleAwareSelection hook if they wanted anyway. This all LGTM

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

@ryanseddon if not intended for this PR, I'd like to see an issue added for breaking out container-utilities. https://github.com/zendeskgarden/react-containers/pull/9/files#diff-f0f75652389df2f7b7d2744cb63a38aeR14

@ryanseddon ryanseddon merged commit 569bf07 into master Feb 27, 2019
@ryanseddon ryanseddon deleted the ryan/selection_container branch February 27, 2019 01:18
Mikaelia pushed a commit that referenced this pull request Dec 14, 2020
BREAKING CHANGE: This version includes the WCAG 2.0 AA compliant "blue" color palette. With this being a large visual change it is a major version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants