search

IHIutch / draft-ui

A collection of simply designed React components focused on making web accessibility as easy as copy & paste.
https://draft-ui.com
MIT License
338 stars 7 forks source link

Breadcrumbs have duplicate role=link #19

Closed Littletonconnor closed 10 months ago

Littletonconnor commented 10 months ago

Breadcrumb items wrap <Link /> but the examples are passing <a> as a child so what ends up happening is you actually get nested anchor tags which I don't think is what's intended.

CleanShot 2024-01-05 at 22 28 40

Happy to raise a PR to fix this if you agree that this is a bug.

Littletonconnor commented 10 months ago

Oh, but if the link is nested within _BreadcrumbItem then we have no way of updating an href

IHIutch commented 10 months ago

Ah, yeah looks like this is a bug caused by a change in the Link component: https://github.com/adobe/react-spectrum/pull/5074

I'm wondering what the simplest way to approach this is... Since Link doesn't inherit its child's props anymore, it's more important that people are able to use the Link prop from their framework of choice. Which means we'll need to handle how Links work in a more global way.

I think the best solution may be to guide folks to wrap their framework's Link with a RAC Link. This way, the styling can be controlled from within Breadcrumb and the logic and prop handling will be controlled by RAC. In theory, this also gives folks a reusable Link component (that they'd probably need/want to do anyway) that combines all the benefits of their framework's routing and the attributes/functionality of RAC.

However, to your point, we may also need to expose something like BreadcrumbLink to allow people to pass an href and continue to handle the link styles internally.

Does that make sense? Anything I'm missing? Btw, I'm planning on spending some significant time tomorrow making some updates so feel free to drop any additional issues or ideas. My plan is to bump RAC to v1 so getting issues like this resolved would be awesome!

IHIutch commented 10 months ago

Actually, as I'm reading more about Link handling, React Aria has some guidance here: https://react-spectrum.adobe.com/react-aria/routing.html

So there may not be a need for a unique Link component, if the RouterProvider is handling this globally. But this probably is something we'll need to document as a part of the installation process because any components using RAC Link won't work as expected without that step.

Littletonconnor commented 10 months ago

So it seems like the best path forward for this component is to expose a BreadcrumbLink to consumers that handle the link styles internally and then add documentation around how to set the RouterProvider globally in order to get to work with your framework of choice. Did I summarize that correctly? If so, I can probably take a stab at this by no later than the end of the day tomorrow if that works for you 😄

IHIutch commented 10 months ago

Yep, sounds good. I wouldn't get carried away with the documentation, I think we should basically reiterate what's already on React Aria. And probably add a link over to their docs as well as a citation/reference. But, if nothing else, the BreadcrumbLink would be awesome.