-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
…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
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.
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" |
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.
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.
It seems to be an expected behaviour according to W3C |
# [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))
🎉 This PR is included in version 1.5.0-next.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [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))
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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

This PR is a bit bigger due to the following changes that were required for the above fix to work:
onClick
prop inCard
component, as click events should be handled by theClickable
component; updated docs and tests respectivelyCard
will no longer render aspan
element witharia-checked="true"
andaria-disabled="true"
whenonClick
prop is not available and the Card is selected - the checked state will be provided by the nestedClickable
component (as long as it has acheckbox
orradio
type)