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

Allows tab function to be used with OL and UL #26650

Closed
wants to merge 5 commits into from
Closed

Allows tab function to be used with OL and UL #26650

wants to merge 5 commits into from

Conversation

tiesont
Copy link
Contributor

@tiesont tiesont commented Jun 2, 2018

There are times when an <ol> is the correct node to use for a list, but the current tab() function will only work with either <ul> or <nav>.

This PR changes ACTIVE_UL to ACTIVE_LIST and adds nodeName === 'OL' to the two checks for the tab button's parent node.

There are times when an `<ol>` is the correct node to use for a list, but the current tab() function will only work with either `<ul>` or `<nav>`. 

This PR changes `ACTIVE_UL` to `ACTIVE_LIST` and adds `nodeName === 'OL'` to the two checks for the tab button's parent node.
Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test which cover this case

@tiesont
Copy link
Contributor Author

tiesont commented Jun 4, 2018

Actually curious to know how the current unit tests pass (assuming v4-dev / js / tests / unit / tab.js is the correct file to update). There are QUnit tests for ol-based markup, but I could not get tabs to work correctly without the changes I've made. I think it's because these tests only check for active and not show, which is how the active tab/pill is indicated. Do I have that right?

@Johann-S
Copy link
Member

Johann-S commented Jun 4, 2018

I think you're right but can you add or change a unit test with a valid example ?

@tiesont
Copy link
Contributor Author

tiesont commented Jun 4, 2018

Sure. Won't be today, but I'll try to add something within a day or two.

@tiesont
Copy link
Contributor Author

tiesont commented Jun 10, 2018

@Johann-S Should the assets/vendor/ folder contain a copy of QUnit (seems so from the README)? Seems to be missing, assuming patch-2 is the correct branch to be working from. Trying to run the unit tests in a browser, since I don't have NPM installed (and would rather not).

Adds OL versions of visual tests and unit tests, with some tweaks to visual tests to indicate which use UL and which use OL.
@Johann-S
Copy link
Member

No we made the decision to remove in our vendors because it's in our dev deps, so you have to install it with NPM, and please do not commit dist files

@tiesont
Copy link
Contributor Author

tiesont commented Jun 10, 2018

Okay, it'd probably be easiest if I close this, delete my fork, and create a new one with just the required updates. Is patch-2 the correct branch to be working from?

@tiesont tiesont closed this Jun 10, 2018
@tiesont tiesont deleted the patch-2 branch June 10, 2018 20:21
@tiesont tiesont restored the patch-2 branch June 10, 2018 20:21
@tiesont tiesont deleted the patch-2 branch June 10, 2018 20:22
@tiesont tiesont restored the patch-2 branch June 10, 2018 20:23
@Johann-S
Copy link
Member

you can call your branch whatever you want 😉

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.

None yet

3 participants