-
Notifications
You must be signed in to change notification settings - Fork 256
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
Expand a11y of CardTopicSelection #527
Comments
CC @bmartins-42 who worked on #276 :) |
Hey @bmartins-42 👋 Just talked this over with a coworker and we have a pretty good idea for this. Likely the best thing would have been to use an input and beneath it a Headless UI tabs component where the component is watching the input. This would allow us to skip from the text input to the options with a press of the tab key and then select options with left and right arrow keys. Beyond this the big thing is that apparently the tabs have an option for them being vertical, so we would just have this option enabled for mobile :) Let's discuss and maybe I can work on this while you look at the modal! |
@andrewtavis following up here from code night :) Happy to look into this :) |
Thanks @TeddyGavi! So happy to have you back and working on this in particular 😊 |
5aac947 just fixes the styles as this is really proving to be difficult. What's missing:
Let me know if you have any thoughts on this, @TeddyGavi! |
I'm starting to wonder if it would be better if we build this from scratch 🤔 Seems like avoiding the default focus behavior of the tabs is gonna be really annoying. |
Hi @andrewtavis, bumping from #628, happy to help out, do you mind outlining the requirements for the issue? Thanks :D |
Hey @UnknownSean8 👋 To outline the goals of this component so we can get on the same page for functionality:
I think that we're running into problems here as I incorrectly thought that the baseline functionalities of Headless UI Tabs could work here. Issue is that we're changing the order of things, and this is really screwing up the a11y that's built into the Headless UI component. |
Also we want this component to shift to a version where the options are |
Some further backstory here, this was originally a combobox like you worked on in #618, but that couldn't work for this as we weren't able to navigate between the options with left and right arrow keys (Headless UI combobox doesn't have an option to do left and right navigation instead of the default up and down). |
Ahh, got it, I'll look into it and let you know. Thanks, @andrewtavis |
FWIW I was also battling to override the default behaviour of the Headless UI Components. I agree it could be simpler to write this by hand. Allowing more control over focus and keyboard navigation. Looking forward to seeing the solution to this 😃 |
I just looked into it just now.
From this, I've tested several cases and it just gives me different results every time. Although I believe there should be some "tricks" to solve each requirement like the ComboBox, writing the component by hand would probably be easier and better fit to the requirements rather than trying to change the Headless UI Tab component itself. I think messing with the component itself will probably just make it less maintainable in the future. Regardless, I will try out some implementations and let me know what are your thoughts, I would love to learn from both of you. :D |
Sounds like we're in agreement on a full rewrite for an amazing result! 😊 Here are some suggestions on structuring the work:
How does this sound to the both of you? From there, @UnknownSean8, what would you like to work on? Happy to set the baseline for it and you can add the functionality (I'd do 1/2 above and maybe 5)? |
Sounds good, @andrewtavis. Thank you! I'll try to implement 3/4 once the baseline is done. Looking forward to it. :D |
Hey @UnknownSean8 and @TeddyGavi 👋 cb57435 reset the functionality of CardTopicSelection and removed the Headless UI components 😊 We're now done with steps 1, 2 and 5 from above, and are ready for 3 and 4! 🚀 Let me know how it's looking, and suggestions of course welcome! |
Hi @andrewtavis @TeddyGavi, I've added the functionality according to the checklist, but the up and down arrow is still pending. It's working as expected, but IMO it's still not the best solution to the problem. Feel free to give any suggestions for the implementation. Also, one problem I found is that the checkbox for the terms and conditions does not check when it's entered when focused. Thanks! |
#648 was just merged in 😊 @TeddyGavi, could you give the component a look over and let us know if you have any final comments on it? Would be great to get your final approval on this as you were the one who envisioned it in the first place :) :) |
@andrewtavis will do! I've been following the conversation and fwiw this is much improved over the Headless Ui approach, in my opinion, good work @andrewtavis and @UnknownSean8 ! 👍 |
Closed by #664 with a minor edge case being handled by #674 :) Thanks so much for all the hard work here, @UnknownSean8! 🎉 Really turned out so well 😊 |
Terms
Description
CardTopicSelection was finished in #276, with overall the component working really really well 😊 One final thing that was noted was that some accessibility improvements could be made. Specifically:
Note that the current implementation uses Headless UI combobox. Moving away from this should be done only if it's impossible to add this functionality via the Vue
<script>
block.Contribution
Happy to support someone who has interest in working on this or get to it myself! 😊
The text was updated successfully, but these errors were encountered: