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

fix(TableToolbarAction): adds forwardRef so focus management works as expected #3918

Merged
merged 10 commits into from
Sep 5, 2019
Merged

fix(TableToolbarAction): adds forwardRef so focus management works as expected #3918

merged 10 commits into from
Sep 5, 2019

Conversation

abbeyhrt
Copy link
Contributor

@abbeyhrt abbeyhrt commented Sep 5, 2019

Closes #2481
Closes #3372

Components using OverflowMenuItem need to have the primaryFocus attribute on the first item so a user can use the arrow keys to go between options. This PR makes it so the README for OverflowMenu, which specifies this, shows up in the react Storybook (#3372 was just confusion around that). It adds forwardRef to TableToolbarAction, in conjunction with adding primaryFocus, so that focus management works as expected.

Changelog

New

  • README visible in storybook for OverflowMenu

Changed

  • adds forwardRef to TableToolbarAction

Testing / Reviewing

  • Go to DataTable with batch actions example, tab into gear and make sure you can use arrow keys to select options.
    Expected:
    Sep-05-2019 11-00-46

@netlify
Copy link

netlify bot commented Sep 5, 2019

Deploy preview for the-carbon-components ready!

Built with commit 52ce5c2

https://deploy-preview-3918--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Sep 5, 2019

Deploy preview for carbon-elements ready!

Built with commit 52ce5c2

https://deploy-preview-3918--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Sep 5, 2019

Deploy preview for carbon-components-react ready!

Built with commit 52ce5c2

https://deploy-preview-3918--carbon-components-react.netlify.com

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.

Fix looks good 👍 I think some snapshots are failing on DataTable, though 👀

@tw15egan
Copy link
Collaborator

tw15egan commented Sep 5, 2019

Do we have any examples using an OverflowMenu in a TableRow? Unable to verify this fixes #3372

@aledavila
Copy link
Contributor

@tw15egan unsure it exists in react. It exists in vanilla, josefina worked on it but I'm not sure who worked on updating the v10 react datatable and if it was included.

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Great! 🏄 Approving, but there is one extra bonus fix if you feel up for it. It's out of the scope of the referenced issues, but since you have a familiarity with the component might be a good add on;

My expectation is that if I have the menu open with an item focused and I hit TAB focus should move to the next tabstop (in this case an "Add New" button). Here's an example using an navigation menu. Tab to the menu, open it, and then hit tab and you'll see focus jump to a link (since the whole navbar itself is a tabstop in the example's case).

The way it works now is the menu closes and focus is returned to the menu's button.

Again, not a blocker and feel free to merge and we can roll this into another ticket 👍

--

UPDATE
Discussed it offline and after some deliberation we're gonna move this to a ticket 💯

@abbeyhrt
Copy link
Contributor Author

abbeyhrt commented Sep 5, 2019

#3372 is a little weird, this fixes it because there was never really a problem in the first place. For the focus management to work, there needs to be the primaryFocus attribute on the first OverflowMenuItem and they weren't doing that. This is in the readme but the readme wasn't in the storybook, this adds it to the storybook. Hopefully people won't have this problem in the future!🤞

@tw15egan
Copy link
Collaborator

tw15egan commented Sep 5, 2019

@abbeyhrt makes sense, thanks for clarifying! Looks good once tests are fixed

@abbeyhrt abbeyhrt merged commit 6dcabd4 into carbon-design-system:master Sep 5, 2019
@abbeyhrt abbeyhrt deleted the modal-user-cant-scroll-3261 branch September 5, 2019 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants