-
-
Notifications
You must be signed in to change notification settings - Fork 79.1k
Dynamic tabs: use buttons rather than links #32630
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
Conversation
- change docs - tweak styles to neutralise border and background
set to draft for now while we wait for #32631 - once that is in place (removing the forced only minor concerns:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include some docs mentions here for why we use <button>
elements here.
- replace links with buttons - make one specific test that uses links instead of buttons, as we still want to support it despite it being non-semantically appropriate - remove tests involving dropdown tabs, as they're discouraged (though still silently supported)
1547c5b
to
34f38ce
Compare
added a short sentence to explain. also took opportunity to update the visual and unit tests |
Put back the two tests that I had originally shelved. This should keep this PR focused and non-breaking |
Leaving these here for now. The test for removed tabs should be redone so that tabs are removed programmatically (as the approach of having that close button inside the link is invalid and broken markup). The test for dropdowns should be removed together we actually ripping out the handling for dropdowns in the tab.js code (arguably a breaking change, though we discouraged this for a few versions and effectively "deprecated" it)
7901d89
to
229a4af
Compare
5010e23
to
1866c27
Compare
1866c27
to
b4a3db8
Compare
🚀 |
Has this been published with Using the exact code from the docs with
Btw. would love to use |
@MickL if you're using the exact same code as in the docs, but it's looking different from https://getbootstrap.com/docs/5.0/components/navs-tabs/#javascript-behavior, then it's likely some other modification/customisation you're doing that's affecting this? if not, please submit a fresh issue with a reduced test case, in case we've missed some nuance of how styles interact with each other in specific conditions |
@patrickhlauke obviously it is exactly the same BUT i changed The issue I opened has been closed in favor of this PR: #32945 |
@MickL Our current docs example (linked by Patrick) is working fine with If you still see any issue related to this in beta 21, please open a new issue—and stop spamming an already merged PR that we cannot re-open. |
this is more semantically correct. note that currently, the forced
:focus
in reboot styles is making this look rather ugly - see separate PR #32631 to remove that cargo culted (?) style from reboot.Preview: https://deploy-preview-32630--twbs-bootstrap.netlify.app/docs/5.0/components/navs-tabs/#javascript-behavior