-
Notifications
You must be signed in to change notification settings - Fork 860
feat(EuiCheckableCard): toggle on children if non-interactive #8907
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(EuiCheckableCard): toggle on children if non-interactive #8907
Conversation
d68821c to
024232f
Compare
| id={`${id}-details`} | ||
| className="euiCheckableCard__children" | ||
| css={childStyles} | ||
| onClick={disabled ? undefined : onChildrenClick} |
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.
I checked the accessibility of this and I think we're good here thanks to the focusable radio input serving the same role.
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.
@tkajtoch Yes, the changes don't affect accessibility because we're not adding a new focusable element. We're only making a non-interactive div clickable (which is generally recommended against but here we have the focusable input that has an accessible name).
| </label> | ||
| {children && ( | ||
| <div | ||
| ref={childrenEl} |
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.
nit: this name confused me for a sec. Could we rename it to childrenWrapperEl? I prefer not to use abbreviations like el, but I'm okay with it here since it just expands on pre-existing code 👍🏻
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.
Yes, I did expand on the pre-existing code and decided to go with the somethingEl naming but actually, we can change it. How would you rename them?
I also agree with childrenWrapper.
|
|
||
| const classes = classNames('euiCheckableCard', className); | ||
|
|
||
| const [interactiveChildren, setInteractiveChildren] = useState< |
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.
Since we're only checking the length of the interactiveChildren array to determine the existence of at least one tabbable child, could we just store it as a binary value here (e.g., hasInteractiveChildren) and save some memory?
I wish tabbable would provide a way to check whether the passed container has any tabbable elements, so it could return on the first match and save even more here.
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.
@tkajtoch ah, you're absolutely right, we can store a boolean. Refactoring...
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.
024232f to
e3b99ac
Compare
tkajtoch
left a comment
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.
Thank you for addressing my comments @weronikaolejniczak! 🎉 LGTM
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
`106.1.0` ⏩ `106.2.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) ## Package updates ### `@elastic/eui` #### [`v106.2.0`](https://github.com/elastic/eui/releases/v106.2.0) - Enhanced `EuiCheckableCard` to make non-interactive children clickable for card selection ([#8907](elastic/eui#8907)) - Added `showFooter` and `toolbarProps.right` props to `EuiMarkdownEditor` for more flexible layout control. ([#8889](elastic/eui#8889)) ([#8889](elastic/eui#8889)) **Bug fixes** - Fixed `EuiPopover` not closing on outside click after multiple fast clicks on the trigger element ([#8882](elastic/eui#8882)) **Accessibility** - Added accessible labels to virtualized `EuiCodeBlock` ([#8887](elastic/eui#8887)) --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]>
`106.1.0` ⏩ `106.2.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) ## Package updates ### `@elastic/eui` #### [`v106.2.0`](https://github.com/elastic/eui/releases/v106.2.0) - Enhanced `EuiCheckableCard` to make non-interactive children clickable for card selection ([elastic#8907](elastic/eui#8907)) - Added `showFooter` and `toolbarProps.right` props to `EuiMarkdownEditor` for more flexible layout control. ([elastic#8889](elastic/eui#8889)) ([elastic#8889](elastic/eui#8889)) **Bug fixes** - Fixed `EuiPopover` not closing on outside click after multiple fast clicks on the trigger element ([elastic#8882](elastic/eui#8882)) **Accessibility** - Added accessible labels to virtualized `EuiCodeBlock` ([elastic#8887](elastic/eui#8887)) --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]>
`106.1.0` ⏩ `106.2.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) ## Package updates ### `@elastic/eui` #### [`v106.2.0`](https://github.com/elastic/eui/releases/v106.2.0) - Enhanced `EuiCheckableCard` to make non-interactive children clickable for card selection ([elastic#8907](elastic/eui#8907)) - Added `showFooter` and `toolbarProps.right` props to `EuiMarkdownEditor` for more flexible layout control. ([elastic#8889](elastic/eui#8889)) ([elastic#8889](elastic/eui#8889)) **Bug fixes** - Fixed `EuiPopover` not closing on outside click after multiple fast clicks on the trigger element ([elastic#8882](elastic/eui#8882)) **Accessibility** - Added accessible labels to virtualized `EuiCodeBlock` ([elastic#8887](elastic/eui#8887)) --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]>
Summary
Resolve #8900
We check whether children are interactive. If no, we trigger the
clickevent on the whole card, including thechildrenwrapper.Why are we making this change?
When using
EuiCheckableCardwith children, e.g.:to select an option, user has to be more precise and click on the radio / checkbox wrapper or the label:
There's a consensus that the expected behavior is if there is static content, clicking on it toggles the radio / checkbox.
Impact to users
🟢 None. All users will benefit from this behavior out-of-box.
QA
Specific checklist
General checklist
Checked in both light and dark modesChecked in both MacOS and Windows high contrast modesChecked in mobileEdge, and FirefoxChecked for accessibility including keyboard-only and screenreader modesAdded documentationProps have proper autodocs (using@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesjest andcypress testsUpdated visual regression testsIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)