-
Notifications
You must be signed in to change notification settings - Fork 192
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
Bug: TokenSelectDropdown is not accessible #744
Comments
Great callout here. NamingI agree with you on the naming. AccessibilityOther than the name rotation: I wanted to improve these but ran out of time. RadixThere were extended conversation about integrating radix. Long story short, the decision was not integrating with radix. However, the intent is to allow users to use UI library of their choice. Overriding UI librarySwapping out underlying components for *uncontrolled components like Token components is straight forward. At most, Token components take three props: Let's dial up the complexity and talk about:
Scenario 1 - Original Swap + Token override componentsOverriding Token component is straight forward. But what if the user wants to use Token override components with Few potential approaches are:
Scenario 2 - Override everythingTake a look at this or latest This PR refactors all business logic to reside at the root Swap component. This allows Swap subcomponents to receive everything via hooks + context. Either all the hooks are exposed or OCK allows component whatever override method used for uncontrolled components.
or something like
I will stop right here but there is quite a few ways to enable to override the entire UI layer. I recommend to explore various methods to achieve the goal of "bring your own UI library". |
To make sure I'm following you correctly. Your goal is to allow users to compose their own components using:
Assuming that's the direction, points 2 and 3 are too overstretched as there are a lot of edge cases that will make things unnecessarily complex and may even confuse users.
I don't see what's the purpose of using onchainkit/src/swap/components/SwapAmountInput.tsx Lines 16 to 24 in c099cc6
That's fine to some extent. The main issue is the level of complexity to re-implement a given feature. If it's too complex, people will give up.
And beyond just using the correct HTML tags, accessibility includes aria attributes, focus, keyboard navigation, screen reader etc... For reference, the Select component from Radix is ~1500 lines without comments. If I were to write my own components, those are not things I would want to handle and it is actually backward when consuming a component library. Before giving complete freedom to users, it would be best for OnchainKit components to be fully functional. Taking
And then expand the scope once things are ironed-out. |
Absolutely agreed there and that's what OCK wants and needs to be.
Agreed there too. OCK approach has been to keep things simple then build it up. Team is very well aware of short comings of current components including full accessibility support.
Absolutely. I do think that OCK can achieve this without being too difficult and actually be straight forward with some good documentation that is copy-paste friendly.
Agreed! OCK does not support UI library override in its current state. It's gonna take some work. Let's breakdown Radix though...
Keep things simple. While radix can solve the At the end of the day, accessibility is the core issue here. Whether to solve that with Radix or not... I wouldn't purely give up on achieving that just because a UI library does it for you. I am taking a long hiatus soon and wanted to give you, and the rest of the team, insight on where my head was at while I wrote up those components. That is all to say, I won't be part of making the decision on how to address the accessibility or on revisiting the decision on Radix integration. I did plan out architecturally how to move forward but I expect things to change given that I won't be here to see it to the end. Lastly, been loving your thoughtful and insightful contributions and questions. Onchain team has been rocking it and hope they continue on with their good work. It's been a pleasure to be part of this journey. |
Thanks for all your insight and stewardship. There's definitely a lot to learn from that. Cheers @kyhyco |
Describe the bug and the steps to reproduce it
The
TokenSelectDropdown
component does not follow the Listbox WAI-ARIA spec https://www.w3.org/WAI/ARIA/apg/patterns/listbox/For example, when the component is open, pressing up/down arrow keys doesn't select values while it should.
Also the component name is not right because it's not a dropdown. Options are just values and don't trigger actions (filter, sort etc...). The component should be renamed to
TokenSelect
.I would suggest to use the Select component from Radix as it already follows the spec. Imo other components should also use Radix primitives as they take care of all the accessibility stuff and allow us to focus on the business logic. Other big libraries like Shadcn are also building on top of it.
I can work on this, just need to make sure we are aligned regarding introducing Radix.
Also I see the Swap component is following what we discussed about in a previous PR. In this case I'll go with an API similar to the Select component from Shadcn.
What's the expected behavior?
Be accessible and have the correct name
What version of the libraries are you using?
0.23.4
The text was updated successfully, but these errors were encountered: