Skip to content

fix: preventing double clicks for the checkboxes#110

Merged
duranmla merged 2 commits intofreenowtech:mainfrom
lloydaf:fix/avoid-double-checkbox-click
Jun 11, 2021
Merged

fix: preventing double clicks for the checkboxes#110
duranmla merged 2 commits intofreenowtech:mainfrom
lloydaf:fix/avoid-double-checkbox-click

Conversation

@lloydaf
Copy link
Contributor

@lloydaf lloydaf commented Jun 8, 2021

What:

​This PR intends to fix an issue with checkboxes, where two clicks are registered instead of one
Why:

​This was causing weird behavior with Google Tag Manager data capturing, and so I decided to investigate. The issue seems to be because of a click being registered on the inner Text component
How:

​Prevent the event from being propogated in order to fix this issue
Media:

https://www.loom.com/share/05dffc1f667d435b9bfdbba4c3dd1ee7
Checklist:

  • Ready to be merged

@snapsnapturtle
Copy link
Contributor

Thanks for contributing. I could think of a few scenarios, which we need to make sure to still work.

  • Checking/Unchecking the checkbox with your keyboard only
  • In tests, query by the label, aka. the text, and click on it to trigger the checkbox to change (we should just write a simple test for this in the unit tests of this component)

@lloydaf
Copy link
Contributor Author

lloydaf commented Jun 8, 2021

@snapsnapturtle looks like the keyboard interactions still work: https://www.loom.com/share/65c1d685b4eb4f768a6724f0d6ac3c75

I'll add a test and update this PR done!

@nlopin
Copy link
Contributor

nlopin commented Jun 9, 2021

Can you describe what was the issue you had?

It is expected that the click event bubbles up. I'd prefer not to break the default behavior

@lloydaf
Copy link
Contributor Author

lloydaf commented Jun 9, 2021

Sure. So the issue was that we were registering two clicks when we clicked on checkboxes, one for the span element that housed the label, and one for the input [type=checkbox] element which is the one we expect to click.

If there is a usecase for emitting click on the label, then I suppose we needn't prevent it from bubbling up, but in my opinion because this is a checkbox, this should be the default behavior (we want only one click when we click on a checkbox, even on its label, instead of two, because its supposed to be one cohesive unit that works as an input element).

@duranmla
Copy link
Contributor

I think this totally makes sense and it is related to #23 isn't?
I guess what we are currently missing is a "focus" state

@lloydaf
Copy link
Contributor Author

lloydaf commented Jun 11, 2021

You can technically check it using your keyboard (like I showed in the video above), but its tricky

@duranmla
Copy link
Contributor

Why is tricky @lloydaf ? cause we don't have the focus or because you have to press multiple times the keyboard?

@lloydaf
Copy link
Contributor Author

lloydaf commented Jun 11, 2021

It's tricky because there's no square box around the checkbox, and so you have to guess if your tab is at the right position. I don't know if that's the focus issue you're talking about but you can verify what I am saying from the video above, no outline when we reach the checkpoint anchors

@duranmla duranmla merged commit 6a1c739 into freenowtech:main Jun 11, 2021
github-actions bot pushed a commit that referenced this pull request Jun 11, 2021
### [1.4.2](v1.4.1...v1.4.2) (2021-06-11)

### Bug Fixes

* **checkbox:** preventing one click trigger twice. one input one for text ([#110](#110)) ([6a1c739](6a1c739))
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.4.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lloydaf lloydaf deleted the fix/avoid-double-checkbox-click branch June 11, 2021 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

5 participants