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

[Fix #17371][V4] Deactivating dropdown links in nav tab #17642

Merged
merged 2 commits into from
Nov 25, 2016
Merged

[Fix #17371][V4] Deactivating dropdown links in nav tab #17642

merged 2 commits into from
Nov 25, 2016

Conversation

matt-hernandez
Copy link
Contributor

This fixes issue #17371

Unit tests are included. If there is any feedback, feel free to let me know.

@haBuu
Copy link

haBuu commented Sep 22, 2015

This bug happens with normal tab links also. Everytime you click a link, active class is added to it and it can't be clicked again because the active class is never removed. Meaning you can only visit every tab once.

@matt-hernandez
Copy link
Contributor Author

@haBuu I'm trying to recreate the behavior you're talking about, but I can't seem to do it. Can you post a JS Fiddle or other example?

@haBuu
Copy link

haBuu commented Sep 22, 2015

http://jsfiddle.net/qz8a1ay5/4/
Having added the tablist role it now works a bit better allowing traversal within tabs if you go to different direction (next/previous). But it still activates links rendering them unuseable. There might also be a error in my markup but it works in v3. Not really sure if something else is needed in v4.

@twbs-lmvtfy
Copy link

Hi @haBuu!

You appear to have posted a live example (https://fiddle.jshell.net/qz8a1ay5/show/light/), which is always a good first step. However, according to the HTML5 validator, your example has some validation errors, which might potentially be causing your issue:

  • line 43, column 3 thru column 8: Start tag body seen but an element of the same type was already open.
  • line 47, column 12 thru column 92: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 48, column 12 thru column 92: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 52, column 12 thru column 92: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 53, column 12 thru column 92: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 57, column 12 thru column 92: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 58, column 12 thru column 92: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 63, column 1 thru column 7: Saw an end tag after body had been closed.

You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!

(Please note that this is a fully automated comment.)

@matt-hernandez
Copy link
Contributor Author

@haBuu I think this is what you were going for: https://fiddle.jshell.net/a_matt_appeared/uvcos7q7/

This is better, but still not working.

In this case, I'm not sure if this behavior will be supported by v4 out of the box. From what I understand, navs are meant to be static objects where there is only one .nav-tabs or .nav-pills per group of tabs.

The use of .nav-pills in this fiddle is more like pagination, and we have three copies. I don't think this is what the BS team intends them to be for.

But if you have an example where this worked in v3, post it.

@twbs-lmvtfy
Copy link

Hi @matt-hernandez!

You appear to have posted a live example (https://fiddle.jshell.net/a_matt_appeared/uvcos7q7/show/light/), which is always a good first step. However, according to the HTML5 validator, your example has some validation errors, which might potentially be causing your issue:

  • line 57, column 3 thru column 8: Start tag body seen but an element of the same type was already open.
  • line 95, column 1 thru column 7: Saw an end tag after body had been closed.

You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!

(Please note that this is a fully automated comment.)

@matt-hernandez
Copy link
Contributor Author

Updated fiddle to satisfy @twbs-lmvtfy

https://fiddle.jshell.net/a_matt_appeared/uvcos7q7/

@haBuu
Copy link

haBuu commented Sep 22, 2015

Here is a working v3 version. http://jsfiddle.net/qz8a1ay5/5/

@twbs-lmvtfy
Copy link

Hi @haBuu!

You appear to have posted a live example (http://fiddle.jshell.net/qz8a1ay5/5/show/light/), which is always a good first step. However, according to the HTML5 validator, your example has some validation errors, which might potentially be causing your issue:

  • line 46, column 9 thru column 87: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 47, column 9 thru column 87: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 51, column 9 thru column 87: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 52, column 9 thru column 87: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 56, column 9 thru column 87: An element with role=tab must be contained in, or owned by, an element with role=tablist.
  • line 57, column 9 thru column 87: An element with role=tab must be contained in, or owned by, an element with role=tablist.

You'll need to fix these errors and post a revised example before we can proceed further.
Thanks!

(Please note that this is a fully automated comment.)

@matt-hernandez
Copy link
Contributor Author

@cvrebert Any feedback on this?

@haBuu Open an issue on this or comment on the referenced issue. I think we're flooding this pull request with too many comments.

@cvrebert
Copy link
Collaborator

@matt-hernandez Not from me. @fat is the one to run this by.

@mdo
Copy link
Member

mdo commented Oct 29, 2016

Would love a review here from someone :).

/cc @Johann-S @bardiharborow (please tell me to stop pinging y'all if this gets too annoying 😆)

@Johann-S
Copy link
Member

For me everything is fine @mdo (and that's ok to ping me don't worry 😃)

@mdo mdo added this to the v4.0.0-alpha.6 milestone Nov 25, 2016
@mdo mdo merged commit b1b1f30 into twbs:v4-dev Nov 25, 2016
@mdo mdo mentioned this pull request Nov 25, 2016
@mdo
Copy link
Member

mdo commented Nov 25, 2016

<3 thanks everyone

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.

7 participants