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

Dynamic tabs: use buttons rather than links #32630

Merged
merged 24 commits into from Feb 9, 2021
Merged

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Dec 27, 2020

  • change docs
  • tweak styles to neutralise border and background

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

- change docs
- tweak styles to neutralise border and background
@patrickhlauke
Copy link
Member Author

set to draft for now while we wait for #32631 - once that is in place (removing the forced :focus style on buttons), this works cleanly.

only minor concerns:

  • .nav-link used on buttons may not seem logical, but changing the class name now would be a breaking change
  • the buttons have black, rather than link-blue, text

@patrickhlauke patrickhlauke marked this pull request as ready for review December 29, 2020 13:40
Copy link
Member

@mdo mdo left a 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.

@patrickhlauke patrickhlauke marked this pull request as draft January 5, 2021 21:36
- 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)
@patrickhlauke patrickhlauke marked this pull request as ready for review January 5, 2021 23:39
@patrickhlauke patrickhlauke requested a review from a team as a code owner January 5, 2021 23:39
@patrickhlauke patrickhlauke requested a review from mdo January 5, 2021 23:39
@patrickhlauke
Copy link
Member Author

We should include some docs mentions here for why we use <button> elements here.

added a short sentence to explain. also took opportunity to update the visual and unit tests

@patrickhlauke patrickhlauke force-pushed the patrickhlauke-navs-tabs branch 4 times, most recently from 5010e23 to 1866c27 Compare February 8, 2021 21:21
@XhmikosR XhmikosR merged commit 96be06e into main Feb 9, 2021
v5.0.0-beta2 automation moved this from Approved to Done Feb 9, 2021
@XhmikosR XhmikosR deleted the patrickhlauke-navs-tabs branch February 9, 2021 05:23
@patrickhlauke
Copy link
Member Author

🚀

@XhmikosR XhmikosR added this to Inbox in v4.6.1 via automation Feb 10, 2021
@XhmikosR XhmikosR moved this from Inbox to Needs manual backport in v4.6.1 Feb 15, 2021
@MickL
Copy link

MickL commented Feb 16, 2021

Has this been published with beta2 6 days ago? For me buttons still dont look good inside nav-tabs and nav-pills:

Bildschirmfoto 2021-02-16 um 14 00 51

Bildschirmfoto 2021-02-16 um 14 02 00

Using the exact code from the docs with button instead of a:

<ul class="nav nav-pills">
  <li class="nav-item">
    <button class="nav-link active" aria-current="page">Active</button>
  </li>
  <li class="nav-item">
    <button class="nav-link">Link</button>
  </li>
  <li class="nav-item">
    <button class="nav-link">Link</button>
  </li>
  <li class="nav-item">
    <button class="nav-link disabled" tabindex="-1" aria-disabled="true">Disabled</button>
  </li>
</ul>

Btw. would love to use button instead of a in tabs in 4.6.x, too! I saw there is a label to backport :)

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Feb 16, 2021

@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

@MickL
Copy link

MickL commented Feb 16, 2021

@patrickhlauke obviously it is exactly the same BUT i changed <a> for <button>

The issue I opened has been closed in favor of this PR: #32945

@ffoodd
Copy link
Member

ffoodd commented Feb 16, 2021

@MickL Our current docs example (linked by Patrick) is working fine with button, this is why this PR got merged and your issue closed.

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.

patrickhlauke added a commit that referenced this pull request Feb 20, 2021
patrickhlauke added a commit that referenced this pull request Feb 21, 2021
@patrickhlauke patrickhlauke moved this from Needs manual backport to Cherry-picked/Manually backported in v4.6.1 Feb 21, 2021
XhmikosR pushed a commit that referenced this pull request Feb 21, 2021
@XhmikosR XhmikosR removed this from Cherry-picked/Manually backported in v4.6.1 Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

Navs and tabs should look the same with <button>
7 participants