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(Card): fix toggle-in-card navigation #165

Merged
merged 9 commits into from
Nov 9, 2022

Conversation

BalbinaK
Copy link
Contributor

@BalbinaK BalbinaK commented Nov 8, 2022

Fixes fabric-ds/issues#113: navigating with Tab, toggling checkbox/radio with "space" key, and navigating between radio toggles with "left" and "right" arrow keys
toggle-in-card-navigation-gif

This PR is a bit bigger due to the following changes that were required for the above fix to work:

  • updated fabric css links to point at v1 to apply focus-ring styles
  • added deprecation warning for onClick prop in Card component, as click events should be handled by the Clickable component; updated docs and tests respectively
  • Card will no longer render aspan element with aria-checked="true" and aria-disabled="true" when onClick prop is not available and the Card is selected - the checked state will be provided by the nested Clickable component (as long as it has a checkbox or radio type)

…asses

Use the latest alias of fabric-ds/css package in the test, documentation and storybook environment
…dered

If Card renders a Clickable radio toggle, and has tabIndex (onClick prop), it breaks
arrow-navigation between other Toggle radio Cards. In order to fix it, the onClick is moved to the
Clickable component and name prop is provided to Clickable radios of the same group.
…Item

Display visual feedback on ToggleItem focus
Click events should be handled by Clickable component, not Card. Before implementing a breaking
change in Card, a deprecation warning is logged if Card receives an onClick prop
@BalbinaK BalbinaK requested a review from a team November 8, 2022 16:09
Copy link
Collaborator

@digitalsadhu digitalsadhu left a comment

Choose a reason for hiding this comment

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

Great stuff.

Question about something I noticed. When you tab to a dead toggle radio group from a previous element it highlights the first item in the radio group. When you then tab past it to another element on the page and tab back with shift+tab, when you hit the radio group, you are on the last dead toggle in the group. Is this as intended?

index.html Outdated
@@ -18,7 +18,7 @@
}
</style>
<link
href="https://assets.finn.no/pkg/@fabric-ds/css/v0/fabric.min.css"
href="https://assets.finn.no/pkg/@fabric-ds/css/v1/fabric.min.css"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, we should really find a way to base this off the version of Fabric CSS listed in package.json. Job for another time though probably.

packages/card/src/component.tsx Show resolved Hide resolved
@BalbinaK
Copy link
Contributor Author

BalbinaK commented Nov 9, 2022

Question about something I noticed. When you tab to a dead toggle radio group from a previous element it highlights the first item in the radio group. When you then tab past it to another element on the page and tab back with shift+tab, when you hit the radio group, you are on the last dead toggle in the group. Is this as intended?

It seems to be an expected behaviour according to W3C

@BalbinaK BalbinaK merged commit fd03fc6 into next Nov 9, 2022
@BalbinaK BalbinaK deleted the 113-fix-toggle-in-card-navigation branch November 9, 2022 12:57
github-actions bot pushed a commit that referenced this pull request Nov 9, 2022
# [1.5.0-next.2](v1.5.0-next.1...v1.5.0-next.2) (2022-11-09)

### Bug Fixes

* **Card:** fix toggle-in-card navigation ([#165](#165)) ([fd03fc6](fd03fc6))
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

🎉 This PR is included in version 1.5.0-next.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Nov 21, 2022
# [1.5.0](v1.4.2...v1.5.0) (2022-11-21)

### Bug Fixes

* **breadcrumbs:** handle array of nodes passed as children ([04728d9](04728d9))
* **Card:** fix toggle-in-card navigation ([#165](#165)) ([fd03fc6](fd03fc6))

### Features

* **toggle:** handle indeterminate state in a select-all checkbox ([#161](#161)) ([af1970b](af1970b))
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

React Toggle problems
2 participants