contentful / forma-36

A design system by Contentful
https://f36.contentful.com
MIT License
329 stars 81 forks source link

refactor: pass pointerEvents none to children of button #2795

Closed massao closed 1 month ago

massao commented 1 month ago

Purpose of PR

In some cases when using SVGs and other elements inside a button the event handler would consider them as being the target of the event, and not the button. By adding the pointer-events: none to the children, we make it so it consistently consider event target being the button.

Also adding an onClick action on the storybook of button to be easier to check.

Before

https://github.com/contentful/forma-36/assets/1071799/d66e1e7c-bdf9-44fa-9009-88d3f8158915

After

https://github.com/contentful/forma-36/assets/1071799/416652cd-4bc1-4a1b-8de7-db6207c30bff

vercel[bot] commented 1 month ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
forma-36 ✅ Ready (Inspect) Visit Preview Jun 12, 2024 2:57pm
changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 02434c109423b775d03fdf42d08bf38d64ae38de

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 36 packages | Name | Type | | ---------------------------- | ----- | | @contentful/f36-button | Patch | | @contentful/f36-accordion | Patch | | @contentful/f36-asset | Patch | | @contentful/f36-autocomplete | Patch | | @contentful/f36-badge | Patch | | @contentful/f36-card | Patch | | @contentful/f36-collapse | Patch | | @contentful/f36-copybutton | Patch | | @contentful/f36-core | Patch | | @contentful/f36-datetime | Patch | | @contentful/f36-datepicker | Patch | | @contentful/f36-drag-handle | Patch | | @contentful/f36-entity-list | Patch | | @contentful/f36-empty-state | Patch | | @contentful/f36-forms | Patch | | @contentful/f36-icon | Patch | | @contentful/f36-header | Patch | | @contentful/f36-list | Patch | | @contentful/f36-menu | Patch | | @contentful/f36-modal | Patch | | @contentful/f36-navbar | Patch | | @contentful/f36-note | Patch | | @contentful/f36-notification | Patch | | @contentful/f36-pagination | Patch | | @contentful/f36-pill | Patch | | @contentful/f36-popover | Patch | | @contentful/f36-skeleton | Patch | | @contentful/f36-spinner | Patch | | @contentful/f36-table | Patch | | @contentful/f36-tabs | Patch | | @contentful/f36-text-link | Patch | | @contentful/f36-tooltip | Patch | | @contentful/f36-typography | Patch | | @contentful/f36-components | Patch | | @contentful/f36-image | Patch | | @contentful/f36-avatar | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 1 month ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
CommonJS 135.8 KB (+0.01% 🔺) 2.8 s (+0.01% 🔺) 69 ms (+84.91% 🔺) 2.8 s
Module 132.07 KB (+0.01% 🔺) 2.7 s (+0.01% 🔺) 80 ms (+70.61% 🔺) 2.8 s
andipaetzold commented 1 month ago

This is a breaking change depending on how F36 users use the IconButton today. Are we sure we want to release this as a patch? Also, in theory, all events should bubble up, so this CSS shouldn't be necessary.

massao commented 1 month ago

This is a breaking change depending on how F36 users use the IconButton today. Are we sure we want to release this as a patch? Also, in theory, all events should bubble up, so this CSS shouldn't be necessary.

We could make the selector SVG specific, what is the main issue, maybe since SVG is a separate HTML object, what makes the event no bubble out from the SVG to the button, that belongs to another HTML object.