-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ui: Keyboard navigation improvements #7090
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. some questions though:
• maybe adding tests for the keyboard events would be nice?
• why not use https://github.com/briarsweetbriar/ember-keyboard ?
• I'd suggest using if
rather than switch
statements with one case
• since this is mostly duplicated, maybe extract to a mixin?
Hey @gregone 👋
Why not indeed! Thanks for that, will take a look! Before looking that addon my comment would be, if it's only a few lines of code to implement the same thing as an addon I'd usually choose the few lines of code. But if this addon is more helpful for this I'll look to switch. I think as well as that, this PR is more of a quick improvement to what we have instead of a full-blown solution. Lemme take a look deeper look at this addon - it looks like something I could potentially use for other things also.
Yeah I did quibble over this myself, and I left it as switch thinking I'd probably add cases later. I think you are right though, for the moment at least its only one case, so I'll switch this out in a sec :bdumtss: 😆
We already have a bunch of 'no-new-mixins' warnings when we start up ember so we are avoiding adding any more mixins and reducing the amount that we already have. I actually tried to see if I could use a helper for this, but you can't seem to return a function from a helper, only strings. In the end I decided that this is fine seeing as its a small improvement and more of a stop gap to adding something more detailed. Actually I wonder if I could use a component with just this action 🤔 , not sure if that would be overkill - definitely food for thought though. |
Hey @gregone So main thing is I switched out the switches and thought a bit more on how I could centralize this little bit of functionality to stop repetition. I almost looked at using a component, but then I thought that modifiers will be available to us soon (once we go full Octane), which would be the ideal tool here, I think. Interestingly modifiers are largely a functional alternative to mixins. All in all I left as is for now as I don't want to take the deep dive into modifiers/Octane just yet, but this will be an area where we can use them when we do. I took a bit of a deeper look at the above addon also, and it also makes very large use of mixins aswell as other soon to be deprecated ember functionality, which we are trying to avoid. So I'm preferring a few lines of code here over the dependency and all that that entails. Let me know what you think, or I'll just go ahead and merge this at the end of today. Thanks again for taking a look! |
@johncowen Very good point about the modifiers. Totally worth waiting for that. It was just an open question, so |
This PR adds 2 things:
{{aria-menu}}
component.html lang="en"
for anyone using a screenreader that has its default set to a non-english language.Oh lastly, I took the opportunity to make some whitespace changes to some templates whilst I was here.