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

enable button toggle on label when checkbox is inside #30388

Merged
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)

…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
@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

@XhmikosR XhmikosR added this to Inbox in v4.5.0 via automation Mar 12, 2020
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 moved this from Inbox to Shipped in v4.5.0 Mar 17, 2020
@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
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v4.5.0
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

5 participants