Skip to content
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

@reach/router Link with accessiblity #3938

Open
HenryKenya opened this issue Nov 15, 2020 · 10 comments
Open

@reach/router Link with accessiblity #3938

HenryKenya opened this issue Nov 15, 2020 · 10 comments

Comments

@HenryKenya
Copy link
Contributor

Issues with Link when using it together with a11y. A lot of props are made default, especially those to do with accessKey and accessKeyLabel.

Is this the expected behavior? @AndrewSouthpaw @pascalduez
Screenshot 2020-11-15 at 12 31 05

@pascalduez
Copy link
Member

Not 100% sure of what's going on, but looking at the libdef looks quite a good path for problems. Spreading a $Shape of a Class in an inexact object...

@HenryKenya
Copy link
Contributor Author

@pascalduez my best guess is that because Link spreads HTMLAnchorElement when using accessibility features by introducing a11y props like accessKey become mandatory

@AndrewSouthpaw
Copy link
Contributor

Hmm looking at a Flow Try it seems like spreading a $Shape only makes the props mandatory if the original type had mandatory props.

Looking at the libdef for HTMLAnchorElement, I'm seeing those props as required. That's probably the issue.

I wouldn't expect to see props like accessKey being required though. It makes me wonder if I'm reading it wrong somehow... either that or it's a bug?

@pascalduez
Copy link
Member

Looking at the libdef for HTMLAnchorElement, I'm seeing those props as required. That's probably the issue.

It's a class.

@pascalduez
Copy link
Member

See #3546

@AndrewSouthpaw
Copy link
Contributor

It's a class.

Woops, makes sense 😅

@HenryKenya
Copy link
Contributor Author

I guess this issue will be fixed with #3546. I'll leave it open for now and close when the PR is merged.

@HenryKenya
Copy link
Contributor Author

Hmm looking at a Flow Try it seems like spreading a $Shape only makes the props mandatory if the original type had mandatory props.

Looking at the libdef for HTMLAnchorElement, I'm seeing those props as required. That's probably the issue.

I wouldn't expect to see props like accessKey being required though. It makes me wonder if I'm reading it wrong somehow... either that or it's a bug?

A side note though @AndrewSouthpaw ..While making props like accessKey required is good for accessibility, wouldn't it be better to have it as a maybe type?

@pascalduez
Copy link
Member

@HenryKenya I'm pretty sure #3546 won't get any attention anymore.
Would you be willing to take it over? Maybe create another MR with the fix, and more importantly, write extensive tests, to make sure it really fix the issue while still working properly.

@HenryKenya
Copy link
Contributor Author

I can work on this over the weekend and push another PR @pascalduez

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants