Skip to content

Conversation

@mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented Jun 12, 2025

Summary

closes #8782

Why are we making this change?

This PR fixes an unexpected behavior where the onChange event would be triggered twice on click of the checkbox within EuiCheckableCard. This would result in the checkbox being unchecked instead of checked.

This was due to the wrapper having a separate click handler to enlarge the clickable area.
The fix applied is to only trigger the wrapper click handler if the wrapper is clicked, not the checkbox.

Screenshots

Scenario: After a single click on the checkbox:

before: checkbox is unchecked

Screenshot 2025-06-12 at 14 55 41

after: checkbox is checked

Screenshot 2025-06-12 at 14 55 44

QA

  • newly added cypress regression test passes
  • checkout this PR and open the added "TESTING_EXAMPLE" story and:
    • verify the checkbox is checked on a single click to the checkbox
    • verify the checkbox is checked on a click to the area around the checkbox and on the label
    • remove the check on the e.target in checkable_card.tsx and confirm that previously the checkbox is not checked on a single click

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in both MacOS and Windows high contrast modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer 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)

@mgadewoll mgadewoll self-assigned this Jun 12, 2025
@mgadewoll mgadewoll marked this pull request as ready for review June 12, 2025 13:53
@mgadewoll mgadewoll requested a review from a team as a code owner June 12, 2025 13:53
@weronikaolejniczak weronikaolejniczak self-requested a review June 12, 2025 17:26
Copy link
Contributor

@weronikaolejniczak weronikaolejniczak left a comment

Choose a reason for hiding this comment

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

LGTM! 🟢 It works as expected, thanks for fixing this so quickly, @mgadewoll!

Non-blocking suggestion:

Let's update our checkable example in the docs to use prevValue => !prevValue to promote React best practices.

packages/website/docs/components/containers/card.mdx:366
onChange={(prevValue) => setCheckbox(!prevValue)}

Screenshot 2025-06-12 at 19 31 05

Copy link
Contributor

@weronikaolejniczak weronikaolejniczak left a comment

Choose a reason for hiding this comment

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

Actually, I checked the staging link and it is not working as expected. Digging in...

Screen.Recording.2025-06-12.at.19.39.59.mov

@mgadewoll
Copy link
Contributor Author

Actually, I checked the staging link and it is not working as expected. Digging in...

@weronikaolejniczak You have a small mistake in your code update, that's why it doesn't work 🙂
It should be () => setChecked(prevValue => !prevValue)

Screen.Recording.2025-06-12.at.19.52.59.mov

Copy link
Contributor

@weronikaolejniczak weronikaolejniczak left a comment

Choose a reason for hiding this comment

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

@mgadewoll The issue was not even that because I did this screen just to send it here, must've typoed 😄 It's that I was reviewing prod and not the staging link. Late PR reviewing 🫠

I reviewed again today and it works as expected both locally in Storybook and in staging docs 👍🏻

@mgadewoll mgadewoll force-pushed the checkable-card/8782-fix-onchange-triggering-twice branch from 20c6835 to 5bd67b7 Compare June 13, 2025 07:27
@mgadewoll
Copy link
Contributor Author

Let's update our checkable example in the docs to use prevValue => !prevValue to promote React best practices.

ℹ️ Updated the docs example in 5bd67b7

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mgadewoll

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mgadewoll

@mgadewoll mgadewoll merged commit 7aa8613 into elastic:main Jun 13, 2025
5 checks passed
acstll added a commit to elastic/kibana that referenced this pull request Jun 19, 2025
`103.0.0` ⏩ `103.1.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)

## Changes

- **EuiDataGrid** now takes an `onFullScreenChange` callback
- An accessibility fix in **EuiComboBox**
- 3 bug fixes

## Package updates

### `@elastic/eui`

## [`v103.1.0`](https://github.com/elastic/eui/releases/v103.1.0)

- Added `onFullScreenChange` prop to `EuiDataGrid` to handle changes
when the component enters or exits fullscreen mode
([#8765](elastic/eui#8765))

**Bug fixes**

- Fixed `onChange` being triggered twice when the checkbox in
`EuiCheckableCard` is clicked
([#8786](elastic/eui#8786))
- Fixed a circular import on the legacy Amsterdam theme that would cause
the theme usage to break
([#8780](elastic/eui#8780))
- Fixed high contrast theme token overrides not being applied
([#8742](elastic/eui#8742))

**Accessibility**

- Fixed form errors not being read by screen readers for `EuiComboBox`
inside of `EuiFormRow`
([#8798](elastic/eui#8798))

<!--ONMERGE {"backportTargets":["8.19","9.0"]} ONMERGE-->
acstll added a commit to acstll/kibana that referenced this pull request Jun 19, 2025
`103.0.0` ⏩ `103.1.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)

## Changes

- **EuiDataGrid** now takes an `onFullScreenChange` callback
- An accessibility fix in **EuiComboBox**
- 3 bug fixes

## Package updates

### `@elastic/eui`

## [`v103.1.0`](https://github.com/elastic/eui/releases/v103.1.0)

- Added `onFullScreenChange` prop to `EuiDataGrid` to handle changes
when the component enters or exits fullscreen mode
([elastic#8765](elastic/eui#8765))

**Bug fixes**

- Fixed `onChange` being triggered twice when the checkbox in
`EuiCheckableCard` is clicked
([elastic#8786](elastic/eui#8786))
- Fixed a circular import on the legacy Amsterdam theme that would cause
the theme usage to break
([elastic#8780](elastic/eui#8780))
- Fixed high contrast theme token overrides not being applied
([elastic#8742](elastic/eui#8742))

**Accessibility**

- Fixed form errors not being read by screen readers for `EuiComboBox`
inside of `EuiFormRow`
([elastic#8798](elastic/eui#8798))

<!--ONMERGE {"backportTargets":["8.19","9.0"]} ONMERGE-->

(cherry picked from commit b0d46f7)

# Conflicts:
#	package.json
#	yarn.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[EuiCheckableCard] onChange handler invoked twice upon checkbox click

3 participants