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

fix(Button): dispatch onclick event on edge #1862

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yaronglp
Copy link
Contributor

@Yaronglp Yaronglp commented Jan 2, 2024

Description

The button doesn't dispatch on click event when clicking on the button's edge.
Issue #1766

Symptoms

Clicking on the Button component causes the edges of the button to move inward before the click event can be fully registered.

Solution

The added margin calculation compensates for the button's size reduction due to scaling.

@SergeyRoyt SergeyRoyt requested a review from a team January 2, 2024 09:17
Comment on lines +77 to +79
--button-scale-factor: 0.95;
transform: scale(var(--button-scale-factor)) translate3d(0, 0, 0);
margin: calc(1% * (1 - var(--button-scale-factor)));
Copy link
Member

Choose a reason for hiding this comment

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

This makes other elements on the screen to be shifted when the button is pressed:

Screen.Recording.2024-01-02.at.11.35.41.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check the issue again.
Do you have a visual testing tool, such as Applitool or Percy?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we use Chromatic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two potential solutions:

  • adding a DOM element as a wrapper
  • replace the scale reduction with box shadow

I don't like both changes because they change the original design or impact performance.

Copy link
Member

Choose a reason for hiding this comment

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

I completely agree, it's a tricky one 💭

@@ -9,7 +9,7 @@ function renderComponent(props) {
return render(<Button {...props}>{text}</Button>);
}

describe("<Buttoon />", () => {
describe("<Button />", () => {
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Thank you @Yaronglp ! That's a really interesting direction, I wonder if we can tweak it a bit so it won't affect other elements

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.

4 participants