💄 [#1495] Make desktop navigation toggle #638
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #638 +/- ##
========================================
Coverage 96.46% 96.46%
========================================
Files 612 612
Lines 21172 21172
========================================
Hits 20424 20424
Misses 748 748 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
617fb8b to
b91ce82
Compare
aa67e32 to
a8a22a9
Compare
| this.setState({ dismissed: false }) | ||
| this.node.classList.add('primary-navigation--open') | ||
| toggleDesktopNavOpen() { | ||
| this.node.classList.toggle('primary-navigation--open') |
There was a problem hiding this comment.
Is .toggle() what is intended in cases like this? Shouldn't it be .add()?
If we .toggle() there is an assumption being made about the state (eg: less robust).
There was a problem hiding this comment.
@Bartvaderkin I'm afraid it really has to be a toggle and I can't really find any warnings about this anywhere - the 'state' isn't really a state here, but one day I might look back on this and think it should be done differently.
| &:not(#{&}--dismissed) &__list-item:focus, | ||
| &:not(#{&}--dismissed) &__list-item:focus-within { | ||
|
|
||
| &:not(#{&}--dismissed)[class*='--open'] &__list-item { |
There was a problem hiding this comment.
Just a note, but selectors like &:not(#{&}--dismissed)[class*='--open'] have that little whiff that triggers the senses.
| @@ -263,12 +269,13 @@ | |||
| &:focus, | |||
| &:focus-visible, | |||
| &:focus-within { | |||
| border: 2px solid orange; | |||
There was a problem hiding this comment.
@jiromaykin misschien moeten we een 'orange' pre-commit githook voor je optuigen :P gefixed in develop aangezien ik er vanuit ga dat dit niet de bedoeling was
There was a problem hiding this comment.
@alextreme dit element heeft een focus-kleur nodig voor toegankelijkheid, maar we hebben daar geen style voor; dus nieuw issue: standaard browser-focuskleur hier in terug brengen of een kleur zetten voor het hele project.
There was a problem hiding this comment.
@jiromaykin Volgens mij hebben we al een focus-kleur, toch? Ik zie 'm in ieder geval wel als ik door de site heen tab. Prima als hiervoor ook op dit element iets wordt toegevoegd, maar dan graag net zoals bij de andere elementen.
(make it work for both main menu and authenticated menu)
(while making Tab and Escape work)
Click outside desktop menu will also close it.