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

Tab.js: Fixes on click handling #33586

Merged
merged 4 commits into from Apr 20, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Apr 8, 2021

  • use prevent default only if triggered by anchor
  • disable auto-initialization if trigger is disabled

Things did:

  • add tests to check event prevent default , and change old ones to meet the isDisabled check during click handling
  • omit isDisabled check on method show

@GeoSot GeoSot requested a review from a team as a code owner April 8, 2021 11:15
@GeoSot GeoSot changed the title Tab.js: Fixes on click Tab.js: Fixes on click handling Apr 8, 2021
@GeoSot GeoSot added this to Inbox in v5.0.0 via automation Apr 14, 2021
@GeoSot GeoSot moved this from Inbox to Review in v5.0.0 Apr 15, 2021
@GeoSot GeoSot moved this from Review to Inbox in v5.0.0 Apr 17, 2021
@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0 Apr 18, 2021
Copy link
Contributor

@alpadev alpadev left a comment

Choose a reason for hiding this comment

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

Is it realistic that someone would initialize tabs by themself and call show() but expecting that nothing happens when the element is disabled? If not then isDisabled on L70 is redundant I'd say because ignoring disabled elements is handled by the click handler now.

Edit: Hm maybe it's not redundant. There would be a way with jQuery to ignore the isDisabled 🤔

Another Edit: Looks like our spec does exactly what I mentioned. Sorry @GeoSot 😅

@GeoSot GeoSot force-pushed the gs-tabs-should-prevent-default-only-in-case-of-anchor branch from ff7f3ef to 3a12793 Compare April 19, 2021 15:46
GeoSot and others added 3 commits April 19, 2021 21:49
* use prevent default only if triggered by anchor
* disable auto-initialization if trigger is disabled
* Remove `isDisabled` check on `show` mehtod, as it is being handled by the click handler
Sorry @GeoSot.. Figured my proposal is breaking our spec and it would be still possible to trigger show via jQuery. Maybe it's better to leave it there even if it's somewhat redundant..
@GeoSot GeoSot force-pushed the gs-tabs-should-prevent-default-only-in-case-of-anchor branch 2 times, most recently from dc97f15 to 9bd99d6 Compare April 19, 2021 18:49
GeoSot pushed a commit to alpadev/bootstrap that referenced this pull request Apr 19, 2021
v5.0.0 automation moved this from Review to Approved Apr 19, 2021
@XhmikosR XhmikosR merged commit 0bbe45c into main Apr 20, 2021
v5.0.0 automation moved this from Approved to Done Apr 20, 2021
@XhmikosR XhmikosR deleted the gs-tabs-should-prevent-default-only-in-case-of-anchor branch April 20, 2021 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants