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

Support for non-list based structure of the nav used with tabs plugin #31443

Closed
wants to merge 8 commits into from

Conversation

rohit2sharma95
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 commented Aug 6, 2020

Find the element with .active class directly to support the non-list based structure (Button toolbar) of the nav used with the tabs plugin.

Closes #31166 and also closes #31392

@rohit2sharma95 rohit2sharma95 requested a review from a team as a code owner August 6, 2020 08:42
@rohit2sharma95 rohit2sharma95 deleted the btn-tabs branch August 21, 2020 10:51
@XhmikosR
Copy link
Member

XhmikosR commented Oct 6, 2020

@rohit2sharma95 feel free to resubmit the PR if it's still valid. Don't take it that we ignore you, it's just that there are periods of time we might not be very responsive.

Regardless, I appreciate taking the time to submit a PR, ❤

@rohit2sharma95 rohit2sharma95 restored the btn-tabs branch October 6, 2020 13:06
@XhmikosR XhmikosR reopened this Oct 6, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Oct 6, 2020

@mdo I'm not sure if we have any limitations coming from the CSS selectors and assuming tests pass we should be OK.

@rohit2sharma95 can you add a test case with the new accepted markup too, which would previously fail?

@rohit2sharma95
Copy link
Collaborator Author

Thanks, @XhmikosR 👍
I will add the required test cases.

@rohit2sharma95
Copy link
Collaborator Author

rohit2sharma95 commented Oct 8, 2020

@XhmikosR and @Johann-S
While I was writing the test case I realized that the markup in this issue #31166 is rare. If they are using .btn-toolbar then I think the .nav class should not be required. They are using:

<div class="nav btn-toolbar mb-3" role="tablist">
  <div class="btn-group mr-2" role="group">
    <a class="nav-link btn btn-secondary active" role="button" data-toggle="tab" href="#home-tab">Home</a>
    <a class="nav-link btn btn-secondary" role="button" data-toggle="tab" href="#profile-tab">Profile</a>
  </div>
  <div class="btn-group" role="group">
    <a class="nav-link btn btn-secondary" role="button" data-toggle="tab" href="#contact-tab">Contact</a>
  </div>
</div>

It is not mentioned in the docs but you need to put either .nav or .list-group class on the parent element to make the tabs working. And in my opinion, if bootstrap is going to support the non-list based structure for the navs then the requirement of the .nav or .list-group should be omitted because the above markup can be written as below and these both classes are not really required 🤔

<div class="btn-toolbar mb-3" role="tablist">
  <div class="btn-group mr-2" role="group">
    <a class="btn btn-secondary active" role="button" data-toggle="tab" href="#home-tab">Home</a>
    <a class="btn btn-secondary" role="button" data-toggle="tab" href="#profile-tab">Profile</a>
  </div>
  <div class="btn-group" role="group">
    <a class="btn btn-secondary" role="button" data-toggle="tab" href="#contact-tab">Contact</a>
  </div>
</div>

There is already an issue about it #31392
It is just my opinion 🙂

@Johann-S
Copy link
Member

@rohit2sharma95

If you can submit a patch which required no Bootstrap classes do not hesitate it would be a good move 👌

js/src/tab.js Outdated Show resolved Hide resolved
@patrickhlauke
Copy link
Member

would be good to check this again, as it's been lingering in limbo for a while. @twbs/js-review

@mdo
Copy link
Member

mdo commented Sep 6, 2021

Adding to v5.2.0—would be rad to get some fresh eyes on this again :).

@mdo
Copy link
Member

mdo commented Dec 28, 2022

Closing as stale, and also other markup is supported here already (per our docs right now).

@mdo mdo closed this Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v5 - tabs required classes [BS4.5] Button toolbar not working with Navs
5 participants