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

feat(checkbox): add onClick prop #9827

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

tay1orjones
Copy link
Member

Closes #9577

Adds an onClick handler to the div wrapping the checkbox input and its span label.

Make note of the code comments I added. I'm curious if there's any additional perspectives on how to solve this issue.

Changelog

New

  • checkbox: add onClick handler

Testing / Reviewing

  • Open the carbon-components-react storybook
  • View the Checkbox Playground story
  • Click a checkbox, 3 events should fire:
    1. onChange
    2. onClick with event.target indicating the span
    3. onClick with event.target indicating the input
  • Click a checkbox label, the same set of 3 events should fire

@netlify
Copy link

netlify bot commented Oct 8, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: a152be4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6165d318a83e840007df0770

😎 Browse the preview: https://deploy-preview-9827--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Oct 8, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: a152be4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6165d3183f454200071cc551

😎 Browse the preview: https://deploy-preview-9827--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 8, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: a152be4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6165d318c478850007375da6

😎 Browse the preview: https://deploy-preview-9827--carbon-elements.netlify.app

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

@tay1orjones Looks good to me! I'm only seeing onChange being called in the Playground but that seems like the same problem as the other PR adding an onClick recently, where the new event showed up locally but not in the deploy preview.

@kodiakhq kodiakhq bot merged commit dd1d037 into carbon-design-system:main Oct 12, 2021
tay1orjones added a commit that referenced this pull request Dec 9, 2021
kodiakhq bot pushed a commit that referenced this pull request Dec 9, 2021
kennylam pushed a commit to kennylam/carbon that referenced this pull request Jul 30, 2024
### Related Ticket(s)

Closes carbon-design-system#9827 

### Description

the `refBottom` needs to be compared to the `containerTop`, so that the menu sticks to the bottom of button

Example (using local `yarn pack`:
https://codesandbox.io/p/devbox/test-overflowmenu-t4slfp?file=%2F.codesandbox%2Ftasks.json%3A1%2C241
live demo: https://t4slfp-9000.csb.app/
### Changelog

**New**

- {{new thing}}

**Changed**

- changed `refBottom` to a let and then equal `refBottom = refBottom - containerTop;`
- ^ wondering if this can be cleaned up code wise, but testing locally with overflow-menu it seems successful

**Removed**

- {{removed thing}}

<!-- React and Web Component deploy previews are enabled by default. -->
<!-- To enable additional available deploy previews, apply the following -->
<!-- labels for the corresponding package: -->
<!-- *** "test: e2e": Codesandbox examples and e2e integration tests -->
<!-- *** "package: services": Services -->
<!-- *** "package: utilities": Utilities -->
<!-- *** "RTL": React / Web Components (RTL) -->
<!-- *** "feature flag": React / Web Components (experimental) -->
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.

Not able to prevent click event propagation on Checkbox/TableSelectRow
3 participants