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.

Sign up for GitHub

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

Jump to bottom

enable button toggle on label when checkbox is inside #30388

Merged
merged 1 commit into from
Mar 16, 2020
Merged

enable button toggle on label when checkbox is inside #30388

merged 1 commit into from
Mar 16, 2020

Conversation

Lausselloic
Copy link
Contributor

fix #30077 for V4 $().button('toggle') not working for checkbox inside label
I' tried with mouse, keyboard and jquery call on label and also click on input
add unit test to avoid further errors

Need to check on V5 without Jquery button plugin have the previous bug (checked state not change)

@Lausselloic
fix twbs#30077 for V4 $().button('toggle') not working for checkbox i…
45b5acb 
…nside label

add unit test to avoid further errors

Need to check on V5 without Jquery button plugin have the previous bug (checked state not change)
@Lausselloic Lausselloic requested a review from a team as a code owner March 12, 2020 15:41
@Johann-S Johann-S requested a review from patrickhlauke March 12, 2020 15:43
@Lausselloic
Copy link
Contributor Author

@patrickhlauke is there a version of your code on the v5? or do I need to make a dedicated PR with your fix and this one?

@XhmikosR
Copy link
Member

I don't think we landed the PR that introduced the issue in master, IIRC

patrickhlauke
patrickhlauke approved these changes Mar 16, 2020
View reviewed changes
Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

Tested in Chrome, Firefox, IE11 with mouse/keyboard/triggering the toggle method programmatically. indeed, this seems to work well.

@XhmikosR
Copy link
Member

@Johann-S LGTY?

@XhmikosR XhmikosR merged commit 8b6dd44 into twbs:v4-dev Mar 16, 2020
@XhmikosR XhmikosR mentioned this pull request Mar 16, 2020
Closed
@mrlife mrlife mentioned this pull request May 18, 2020
Closed
@mdo
Copy link
Member

mdo commented May 21, 2020

Possible regression in #30849?

@XhmikosR
Copy link
Member

Seems like it. @Lausselloic can you have a look please and CC me? This will probably need to PRs one if the issue is present in master too.

@XhmikosR
Copy link
Member

XhmikosR commented Jun 9, 2020

@Lausselloic friendly ping :)

@Lausselloic
Copy link
Contributor Author

Sorry I've miss the ping. Yes it's due to that PR. I will give a try to fix it.

@Lausselloic Lausselloic mentioned this pull request Jun 9, 2020
Merged
@Lausselloic
Copy link
Contributor Author

@XhmikosR I've update the solution. Instead of stopping the event propagation if event is fired on checkbox inside the label, I detect if the event is not on this element before firing the toggle.

Tests are okay for I'll let js team check that don't break other stuff but looks better for me

@nelsonni nelsonni mentioned this pull request Jan 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Johann-S Johann-S Johann-S approved these changes

@patrickhlauke patrickhlauke patrickhlauke approved these changes

Assignees
No one assigned
Labels
js v4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Lausselloic @XhmikosR @mdo @patrickhlauke @Johann-S