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

[docs] Better react-router-dom version comment #16335

Merged
merged 4 commits into from
Jun 23, 2019

Conversation

kyarik
Copy link
Contributor

@kyarik kyarik commented Jun 22, 2019

The comment in the linked issue specifies version 5 of react-router-dom and not 6. In fact, there is no version 6 or greater. At this time, the current version of react-router-dom is 5.0.1. So, I updated the comment to say react-router-dom < 5.0.0

The comment in the linked issue specifies version 5 of `react-router-dom` and not 6. In fact, there is no version 6 or greater. At this time, the current version of `react-router-dom` is 5.0.1. So, I updated the comment to say *react-router-dom < 5.0.0*
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 22, 2019

No bundle size changes comparing 0c8d847...94732b1

Generated by 🚫 dangerJS against 94732b1

@kyarik kyarik changed the title Fixed react-router-dom version in comment [docs] Fixed react-router-dom version in comment Jun 22, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 22, 2019

The version is correct. The usage of forwardRef is still required with react-router v5. The ref prop can't be used, it has to be innerRef.

@oliviertassinari oliviertassinari changed the title [docs] Fixed react-router-dom version in comment [docs] Better react-router-dom version comment Jun 22, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 22, 2019

I have updated the pull request with a different wording. Does it help? :)

@oliviertassinari oliviertassinari added component: link This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation labels Jun 22, 2019
Copy link
Member

@joshwooding joshwooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage of React.forwardRef might no longer be required for react-router-dom v6.

@kyarik
Copy link
Contributor Author

kyarik commented Jun 23, 2019

Yes indeed, after reading more comments in the linked issue, it's evident that react-router-dom v5 doesn't yet support React.forwardRef, but it might support it in v6.

However, I am not sure whether linking to that issue comment is useful. It seems misleading because it appears to say that React.forwardRef will be supported in v5 (and that's why I created this pull request in the first place). Wouldn't it be better to link to this comment instead?

@oliviertassinari oliviertassinari merged commit 6290988 into mui:master Jun 23, 2019
@oliviertassinari
Copy link
Member

@kyarik It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@kyarik
Copy link
Contributor Author

kyarik commented Jun 23, 2019

@oliviertassinari What a timing! You merged this right when I was about to post the above comment :)

@oliviertassinari
Copy link
Member

Interesting, I would hope that linking an open issue is enough.

@kyarik
Copy link
Contributor Author

kyarik commented Jun 23, 2019

I think the link should be either to the entire issue or to this comment because otherwise you say

The usage of React.forwardRef will no longer be required for react-router-dom v6
see remix-run/react-router#6056 (comment)

But that linked comment appears to imply that it is supported in v5, which contradicts the above sentence.

@oliviertassinari
Copy link
Member

👍

@kyarik
Copy link
Contributor Author

kyarik commented Jun 23, 2019

Shall I create the pull request?

@oliviertassinari
Copy link
Member

Don't worry about it, I batch small changes every now and then (after 1 week or 10 items). It will be updated to only link the issue.

@kyarik kyarik deleted the patch-1 branch June 23, 2019 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: link This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants