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 requested a review from patrickhlauke April 14, 2021 21:25
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants