Tabs/Scrollspy/.nav/.list-group/.active independent of markup (<nav>, .nav-item, <li> etc...) #21807

pvdlg · 2017-01-21T22:47:51Z

Fixes #20451, fixes #19736, fixes #21223, fixes #18566, fixes #18409, fixes #20620, fixes #20156.

The intent in v4 is to support two type of markup for .nav, tab, scrollspy:

The "<ul> markup":

<ul class="nav">
  <li class="nav-item">
    <a class="nav-link" href="#">Item</a>
  </li>
</ul>

The "<nav> markup":

<nav class="nav">
    <a class="nav-link" href="#">Item</a>
</nav>

This PR provide the necessary fixes:

Make scrollspy works with both markup. It is not dependent of .nav-item or <li> anymore
Add corresponding unit tests
Fix .active .nav-link for both markup broken here
Fix .nav-justified for both markup that was broken for "<nav> markup"
Fix .nav-fill for both markup that was broken for "<nav> markup"
Clarify scrollspy docs to mention both markup and that .nav-link receives the .active class
Add a new scrollspy example in the docs with nested .nav-links
Make tabs scss works with both <nav> and <ul> markup
Make tabs plugin works with both markup. It is not dependent anymore of .nav-item or <li>, <a> etc...
Revert PR Allow to use Tab.js with list-group #21756 to handle list-group with tabs in a more simple way with less specific class dependencies
Fix tabs pane transition detection that was using an unnecessary additional selector
Add example in docs for Tabs with JS with both markup
Add examples for tab pills and tab pills vertical

In addition it adds the support of scrollspy and tab to 'list-group (per request in #20620):

Change tab plugin to support .list-group
Add tab example and documentation in list-group page
Add unit test
Change scrollspy plugin to support .list-group
Add scrollspy example and documentation
Add unit test

Both tab and scrollspy plugin now only depends on:

Having.nav or 'list-group class on the parent or sub-parent in case of nested navs
Having .nav-link or .list-group-item anywhere within to add the .active
No other markup structure / element / class is necessary, allowing the maximum of flexibility

…f nav-item class or of the markup elements (li, nav, div etc…)

…dd an example with sub-nabs

wojtask9 · 2017-01-22T18:09:49Z

scss/_nav.scss

@@ -76,13 +82,6 @@
  .nav-link {
    @include border-radius($nav-pills-border-radius);
  }
-


Not 100% sure but this change breaks navs with dropdowns.

Are you talking about this one?

&.active { color: $nav-pills-active-link-color; cursor: default; background-color: $nav-pills-active-link-bg; }

Or that one?

.nav-link { @include border-radius($nav-pills-border-radius); }

For info the second one wasn't modified in this PR.

Anyway in both cases I can't reproduce the problem you are reporting regarding navs with dropdown.
Could you clarify in which way it breaks it ? Or point me to an exemple in the docs that is broken? Or provide more details?

My bad, I see what you means now. I fixed it here. Thanks !

Johann-S · 2017-03-27T11:21:51Z

Maybe @vanduynslagerp can rebase his branch (instead of a merge) to fix the build issue

Johann-S · 2017-03-29T08:46:13Z

Sorry about that @vanduynslagerp but can you update your branch again 😄 thank you 👍

pvdlg · 2017-03-29T14:12:12Z

Done

Johann-S · 2017-03-29T14:13:39Z

Thank I'll review your PR asap 👍

Johann-S

Can you update this file : https://github.com/twbs/bootstrap/blob/v4-dev/js/tests/visual/tab.html#L199

Because in your branch it doesn't work the same as v4-dev

pvdlg · 2017-03-29T14:36:29Z

Can you update this file : https://github.com/twbs/bootstrap/blob/v4-dev/js/tests/visual/tab.html

Sure. I somehow always forget the visual tests. I'll do it tonight (EST time).

Johann-S · 2017-03-29T14:37:45Z

Sorry for the lack of details, the visual test about tabs with list group doesn't work same as v4-dev

Johann-S · 2017-03-29T19:30:10Z

docs/components/list-group.md

+</div>
+
+{% highlight html %}
+<div class="row">


I think you can remove this div (<div class="row">) from your example

Do you mean to make the sample code more concise ?

Yes exactly

Ok. Then I'll remove <div class="col-4"> as well I suppose. I thought it would be helpful for the user to see how to make the vertical menu with the grid. But thinking about it now, it's true it makes the sample bigger and it's not really the place for that anyway. I'll change it tonight

Johann-S · 2017-03-29T19:43:40Z

docs/components/scrollspy.md


-Scrollspy currently requires the use of a [Bootstrap nav component]({{ site.baseurl }}/components/navs/) for proper highlighting of active links.
+<div class="bd-example">


I cannot see this example working because they aren't scroll and when I scroll the main scroll no active class added

Works for me on Safari, Chrome and Firefox.
The scroll is set by the class .scrollspy-example-2 I added to docs/assets/scss/_component-examples.scss. I guess you have to recompile the docs with grunt docs.

Yes I thought about it after my comment my bad 👍

Johann-S · 2017-03-29T19:45:19Z

docs/components/scrollspy.md

+
+The ScrollSpy plugin also works with a `.list-group`. Scroll the area next to the list group and watch the active class change.
+
+<div class="bd-example">


Same with this example no active class added

Shorten some examples

pvdlg · 2017-03-30T05:43:35Z

I updated the visuals tests and removed the row and col from doc examples.
The problematic visual test come from you PR #21756. I reversed all your code but I forgot the visual test. It's reversed and fixed.

A few explanation regarding the reversal of your code.
In #21756 you were referencing explicitly the list-group-item and applying active on the parent.
A few problems with that:

It makes the plugin less generic as you reference a specific class (list-group-item) and it wasn't necessary. Without that, users can use any classes for the actionable item.
You broke the module "pattern" which is to set active on the element that activated the tab (i.e the a on which the user click which make semantic sense). You were setting that on the parent.
You were trying to handle a markup that is not supported according to the doc (in case of an actionable list item the a has to be the child of the list-group like in list-group/#links-and-buttons. Which explain V4 list-group with an active li, invisible link #21755 I imagine.

In the tabs visual the JS of my PR was actually working (the active class was changed properly) as your code were using the markup for tabs (with the actionable item wrapped in other element, itself child of the nav/list-group). The problem was the css as explain above, as it expect the active element as a child of the list-group.
Also in your visual test you had two tab-panel with exactly the same paragraphs, so switching tabs was not visible (it was actually changing but displaying the exact same text).

Johann-S · 2017-03-30T08:44:04Z

Thank you @vanduynslagerp 👍 I'll review your last changes later

Johann-S

(I runned grunt docs)
On the list group no active class added here (but it works fine on the visual test)

Here I think a scroll is missing and no class actives

pvdlg · 2017-03-30T20:57:51Z

Somehow you must not have the right version of the code. The scroll is created by the class here. So it seems you don't have a local documentation generated from the code of my branch.

Make sure you have pulled all the code from my branch then run:

grunt
grunt doc
bundle exec jekyll server

Johann-S · 2017-03-31T07:15:01Z

I was up to date before you last commit and I did the grunt command you said.
But I'll try again later

Johann-S · 2017-03-31T19:39:38Z

Everything is fine but your branch is not up to date @vanduynslagerp

* tweak some copy * fix up scrollspy docs * remove nav styles that were added * fix nav-based docs by requiring .nav-item on .nav-link

pvdlg added 10 commits January 21, 2017 17:25

Fix active nav-link for nav markup

3bff97c

Fix nav-justified for nav markup

9fe3960

Fix nav-fill for nav markup

5b471f2

Add flex-fill utility that is referenced in the doc but not implemented

df3dfc2

Add font-size utility used in the scroll spy docs

536aefe

Detect parent to activate for both ul and nav markup, independently o…

1aee9d1

…f nav-item class or of the markup elements (li, nav, div etc…)

Remove unused variables

16e7a05

Unit test for both <ul> and <nav> markup

98bbe1d

Update nav docs to demonstrate nav-justified + <nav> markup

009da31

Clarify dependency to nav, mentioned both <ul> and <nav> markup and a…

2555b9e

…dd an example with sub-nabs

This was referenced Jan 21, 2017

Scrollspy broken when using non-list-based navbar markup #19736

Closed

Scrollspy and active item/link #20451

Closed

pvdlg changed the title ~~Make scrollspy and nav markup independent~~ Make scrollspy and nav independent of markup (<nav>, .nav-item, lie etc...) Jan 21, 2017

wojtask9 reviewed Jan 22, 2017

View reviewed changes

pvdlg added 6 commits January 22, 2017 15:00

Display dropdown as active when dropdown shown

29e71b7

Make tabs works with both <nav> and <ul> markup

0c24034

Find active tab / tab-pane independently of markup

6fdfbd5

Fix tab-pane transitioning detection

463caee

Remove unnecessary variables

d69bb07

Add example with <nav> markup and JS

0515419

pvdlg changed the title ~~Make scrollspy and nav independent of markup (<nav>, .nav-item, lie etc...)~~ Tabs/Scrollspy/.nav/.active independent of markup (<nav>, .nav-item, lie etc...) Jan 22, 2017

pvdlg added 2 commits January 22, 2017 16:18

Fix html markup in docs

47c439a

Fix more markup

753a6f5

pvdlg changed the title ~~Tabs/Scrollspy/.nav/.active independent of markup (<nav>, .nav-item, lie etc...)~~ Tabs/Scrollspy/.nav/.active independent of markup (<nav>, .nav-item, <li> etc...) Jan 22, 2017

pvdlg mentioned this pull request Jan 22, 2017

Tab JS and .active class #21223

Closed

7 tasks

pvdlg added 5 commits January 22, 2017 18:20

Detect tabs list based on class .nav rather than elements

3664142

Merge branch 'twbs/v4-dev' into nav-scrollspy

7a7ae03

Fix aria-control

c5cebd2

Rename NAV selector

2392e9b

Add tab pills and vertical tab pills examples

4afc8c0

Merge branch 'v4-dev' into nav-scrollspy

f3e2886

Merge branch 'v4-dev' into nav-scrollspy

e8b5f79

Johann-S requested changes Mar 29, 2017

View reviewed changes

Johann-S approved these changes Mar 29, 2017

View reviewed changes

Johann-S self-assigned this Mar 29, 2017

pvdlg added 2 commits March 30, 2017 00:30

Reverse 1a46d8c

2ade5e7

Update visual test for scroll spy and tab

3227b7b

Shorten some examples

Johann-S requested changes Mar 30, 2017

View reviewed changes

Fix copy/paste error in highlighted html (the actual code didn't change)

1996643

Johann-S approved these changes Mar 31, 2017

View reviewed changes

Merge branch 'v4-dev' into nav-scrollspy

eb7d9e0

Johann-S merged commit 91b6294 into twbs:v4-dev Apr 2, 2017

Johann-S added this to the v4.0.0-beta milestone Apr 2, 2017

This was referenced Apr 2, 2017

Beta 1 ship list #21568

Closed

Docs and CSS updates for #21807 #22336

Merged

mdo added a commit that referenced this pull request Apr 8, 2017

Docs and CSS updates for #21807 (#22336)

9a47573

* tweak some copy * fix up scrollspy docs * remove nav styles that were added * fix nav-based docs by requiring .nav-item on .nav-link

BranTurner mentioned this pull request Jun 11, 2017

Temporarily fix Bootstrap tabs (#26) monicahq/monica#200

Merged

MartijnCuppens mentioned this pull request Feb 6, 2019

Fix .nav-justified example #28193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tabs/Scrollspy/.nav/.list-group/.active independent of markup (<nav>, .nav-item, <li> etc...) #21807

Tabs/Scrollspy/.nav/.list-group/.active independent of markup (<nav>, .nav-item, <li> etc...) #21807

pvdlg commented Jan 21, 2017 •

edited

Loading

wojtask9 Jan 22, 2017

pvdlg Jan 22, 2017

pvdlg Jan 22, 2017 •

edited

Loading

Johann-S commented Mar 27, 2017

Johann-S commented Mar 29, 2017

pvdlg commented Mar 29, 2017

Johann-S commented Mar 29, 2017

Johann-S left a comment •

edited

Loading

pvdlg commented Mar 29, 2017

Johann-S commented Mar 29, 2017

Johann-S Mar 29, 2017

pvdlg Mar 29, 2017

Johann-S Mar 29, 2017

pvdlg Mar 29, 2017 •

edited

Loading

Johann-S Mar 29, 2017

pvdlg Mar 29, 2017

Johann-S Mar 29, 2017

Johann-S Mar 29, 2017

pvdlg commented Mar 30, 2017

Johann-S commented Mar 30, 2017

Johann-S left a comment

pvdlg commented Mar 30, 2017

Johann-S commented Mar 31, 2017

Johann-S commented Mar 31, 2017


		Scrollspy currently requires the use of a [Bootstrap nav component]({{ site.baseurl }}/components/navs/) for proper highlighting of active links.
		<div class="bd-example">


		The ScrollSpy plugin also works with a `.list-group`. Scroll the area next to the list group and watch the active class change.

		<div class="bd-example">

Tabs/Scrollspy/.nav/.list-group/.active independent of markup (<nav>, .nav-item, <li> etc...) #21807

Tabs/Scrollspy/.nav/.list-group/.active independent of markup (<nav>, .nav-item, <li> etc...) #21807

Conversation

pvdlg commented Jan 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdlg Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Johann-S commented Mar 27, 2017

Johann-S commented Mar 29, 2017

pvdlg commented Mar 29, 2017

Johann-S commented Mar 29, 2017

Johann-S left a comment • edited Loading

Choose a reason for hiding this comment

pvdlg commented Mar 29, 2017

Johann-S commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdlg Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdlg commented Mar 30, 2017

Johann-S commented Mar 30, 2017

Johann-S left a comment

Choose a reason for hiding this comment

pvdlg commented Mar 30, 2017

Johann-S commented Mar 31, 2017

Johann-S commented Mar 31, 2017

pvdlg commented Jan 21, 2017 •

edited

Loading

pvdlg Jan 22, 2017 •

edited

Loading

Johann-S left a comment •

edited

Loading

pvdlg Mar 29, 2017 •

edited

Loading