Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add <Focus> component to react-router-dom #6449
Add <Focus> component to react-router-dom #6449
Changes from 4 commits
7b03561
d097b16
fe5dd4d
102d84c
126dc03
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If an element isn't natively focusable, could we perhaps do a
cloneElement
and add thetabIndex="-1"
automatically behind the scenes?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.
I don't think that
cloneElement
would work with this API because we don't necessarily have access to the React element. We do have aref
to a DOM element, so the attribute could be to the element withref.setAttribute("tabindex", "-1")
.My personal preference would be to require the user to set the tab index. It's a little more work for the dev than RR injecting the attribute, but the
ref
warning will let them know if there is an issue with their code and it lacks any "magic" (being able to focus an element that shouldn't be focusable).End of the day, (assuming setting the attribute works like I think it would) I can make the switch if you would prefer.
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.
Should we add some
propTypes
to the<Focus>
component here, like we do elsewhere?I'm kinda on the fence about
propTypes
personally these days, since so much of the community is moving away from them and they just increase our bundle size. But I think at least for 4.x we should keep using them on public API.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.
You can build your modules with a Babel plugin that strips prop types in production mode. That's what I do for RNWeb
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.
I meant to do this. This code was adapted from TypeScript, so I stripped out all of type references and then completely forgot to add prop types.
I would not miss prop types.
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.
Thanks for the tip, @necolas :) We used to use that plugin, but we use the
__DEV__
flag now instead, just so it's really clear when looking at the code which stuff is included in dev and which stuff isn't.When I mentioned bundle size, what I meant was that I'm not sure if our
prop-types
import is able to be dropped in our production builds, because it isn't conditional. We aren't actually using prop types in our production code, so in theory it should be tree-shaked out of our production bundles, but we have a brand new bundling process in 4.4 (thx to @TrySound) and I still need to verify.