navbar - resolve some a11y issues #188
Conversation
MelSumner
changed the title
There was a problem hiding this comment.
There are a lot of comments in my review but that shouldn't give you the impression that I don't like it 🙈
This is a massive improvement to the code that we had before!! I am so happy that you took a look at this.
There seem to be some issues with it when you click one dropdown and then another (see my point about registering dropdowns in the service).
Let me know if you want me to take a look and maybe implement some of my suggestions 👍
| links.forEach((ancor) => { | ||
| ancor.addEventListener('blur', () => this.handleBlur()); | ||
| }); | ||
| navbar: service(), //TODO also do we need this too? |
There was a problem hiding this comment.
yes we do (I think anyway). It was used before to register any navbar links and then when we open a dropdown (triggerDropdown) we close all others.
I haven't checked if your implementation needs it any more 🤔 Although it might do because if you click one dropdown after another on this page https://deploy-preview-188--ember-styleguide.netlify.com/components/navbar they stay open. I can take a look at that if you like?
| z-index: 100; | ||
| } | ||
| } |
There was a problem hiding this comment.
a lot of this file seems to be indented by 2 spaces (but not nested in anything 🤔) is that intentional?
| <span></span> | ||
| </div> | ||
| </form> | ||
| {{/if}} |
There was a problem hiding this comment.
see my comment above, we should probably delete this
|
@mansona we'll have to figure out some of this font sizing stuff- the field guide is great but the practical implementation is resulting in some odd accessibility things. I almost wonder if we write it then abstract the values, instead of vice versa? I'm not sure, let's make sure we talk about it. |
|
@mansona can we schedule some time to go through what this needs to be merged? |
|
won't be used, closing PR. |
Updated to add screenshot of progress: