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 does not dispatch onClick event on edge click #1978

Closed
wants to merge 1 commit into from
Closed

fix: Button does not dispatch onClick event on edge click #1978

wants to merge 1 commit into from

Conversation

RitikBora
Copy link

Hey @talkor ,

This PR is raised in regards to the issue pointed out in #1766 .

Based on our discussion, there is a very slight diffrence in the onClick and onMouseDown events . Hence, I have fired an onClick event whenever a mouseDownEvent occurs. It takes care of the button edge clicks as well as multiple clicks as everytime a mouseDown event is surely registered

Thanks,
Ritik

@RitikBora RitikBora requested a review from a team as a code owner February 21, 2024 14:56
@RitikBora RitikBora changed the title onClick event fired on mouseDown fix for Button does not dispatch onClick event on edge click Feb 21, 2024
@RitikBora RitikBora changed the title fix for Button does not dispatch onClick event on edge click fix: Button does not dispatch onClick event on edge click Feb 21, 2024
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.

@RitikBora I appreciate it, but I didn't mean it's a change we should make in the component but a workaround to avoid the bug where button clicks on the edges do no trigger onClick when the animation is on. We want to allow both onClick and onMouseDown for the consumer of the component to decide which to use according to their needs.

@RitikBora
Copy link
Author

Hey @talkor ,

Oh Okay. The change i made wouldn't not hamper the onMouseDown functionality at all. It just fires the onClick event as well.

I'm happy to pullback my request and look into some better solution than a walkaround :) . Let me know if you are ok with that..

Cheers.

@talkor
Copy link
Member

talkor commented Feb 21, 2024

Hey @talkor ,

Oh Okay. The change i made wouldn't not hamper the onMouseDown functionality at all. It just fires the onClick event as well.

I'm happy to pullback my request and look into some better solution than a walkaround :) . Let me know if you are ok with that..

Cheers.

Since it's a breaking change (users who called both onClick and onMouseDown will have onClick called twice for example) we wouldn't want to push this change at the moment, especially if it's meant to fix a different minor bug. I suggest to focus on trying to resolve the original bug without affecting the API for now. If you have an idea we'd be more than happy. Thanks!

@RitikBora
Copy link
Author

Hey @talkor ,

Understood,

Thanks!!

@RitikBora RitikBora closed this Feb 21, 2024
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.

2 participants