Improve class selectors for navigation #2852
Conversation
|
@bartaz out of curiosity. What is the benefit of moving the styles into one use placeholders? I tend to try and put the code where a developer would look first for something. |
|
@anthonydillon These parts will be duplicated for new class names and current deprecated ones. Not to duplicate the code, I moved it to placeholders so both new and old class names can extend from it. Old ones will be wrapped in |
bartaz
changed the title
|
Makes sense 👍 forgot about the wrapping deprecate mixin. Thanks for explaining. |
Deprecates p-navigation__links, p-navigation__link and > a selectors. Updates default example to use new classes, adds new deprecated example to make sure we keep support.
bartaz
force-pushed
the
navigation-cleanup
branch
from
307e9e9
to
84f100b
Compare
bartaz
changed the title
|
@bartaz would it be scope creep to fix the p-subnav situation? Shouldn't |
|
@lyubomir-popov I agree that we should fix subnav, but this PR got quite complex even without it. I created #2857 so we have this in backlog. |
| Closes all subnavs on the page. | ||
| */ | ||
| function closeAllSubnavs() { | ||
| var subnavs = document.querySelectorAll('.p-subnav'); |
There was a problem hiding this comment.
I wonder if its ever likely to have more then one on a page at a time? In that case we could limit the search to the used subnav.
There was a problem hiding this comment.
Yes. Why wouldn't you have multiple dropdowns in the navigation? We have this in our example where each item in menu is a dropdown.
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
|
|
||
| var subnav = subnavToggle.parentElement; |
There was a problem hiding this comment.
Maybe use closest so the JS doesnt need to change if the markup does
There was a problem hiding this comment.
closest is not supported by IE. We would need to polyfill that.
|
|
||
| In Vanilla 2.8 we deprecated the use of `p-navigation__links`, `p-navigation__link` and classless `<a>` in the navigation. Support for these classes will be removed in future version 3.0. | ||
|
|
||
| You should use `p-navigation__items`, `p-navigation__item` and `<a class="p-navigation__link">` instead. |
There was a problem hiding this comment.
Maybe we should start a 3.0 upgrade document now so its easier when it comes to it?
There was a problem hiding this comment.
Created issue about it #2858
| @extend %navigation-link-dark; | ||
| } | ||
|
|
||
| .p-navigation__item.is-selected > .p-navigation__link { |
There was a problem hiding this comment.
Do we need the sibling selector now that we are directly targeting classes?
There was a problem hiding this comment.
Yes. In this case it depends on is-selected state of the parent. I guess we could simplify it to .p-navigation__item.is-selected .p-navigation__link if that's what you mean.
| padding-right: 2 * $sph-inner--small + map-get($icon-sizes, default); // icon padded with the default padding-right of selects, inputs etc. | ||
| } | ||
|
|
||
| .p-subnav > .p-navigation__link { |
There was a problem hiding this comment.
Seems strange to cross use two patterns here?
There was a problem hiding this comment.
It's been like this before. I don't want to change it in this PR. I opened a separate issue to move p-subnav into navigation #2857.
anthonydillon
added
Review: Code +1
Review: QA +1
and removed
Review: Code needed
Review: QA needed
labels
|
Not really a breaking change 🏅 |
@anthonydillon Yes, when we deprecate things there are never really breaking changes (at least not intentionally). I use this label to remind myself to mention the deprecated classes in release notes. |
Done
p-navigation__items,p-navigation__item,p-navigation__link) to be used in place of existingp-navigation__links,p-navigation__linkand classless<a>Fixes #2168
Fixes #1746
QA
./runPlease note - embedded CodePen examples will not work because they use published version of Vanilla, not latest one in development.