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

Document correction - update ReactRouter3.md #299

Merged

Conversation

Carr1005
Copy link
Contributor

@Carr1005 Carr1005 commented Sep 21, 2021

First, thanks for building such a useful HOC! I know react-router-3 is quite aged and may rarely be used in the community today, but I believe there must still be some who would like to adopt this library to save their life like me. 🙌

The changes are the same as the commit message:

  1. Correct the code logic in the example of userIsNotAuthenticated.
  2. Align minor inconsistencies in code examples.

FYI: The doc on Gitbook needs updates too.


Also here is a proposal, would like to hear how the author and maintainers of this repo think:
I think the name of property authenticatedSelector in connectedRouterRedirect could be changed to a more neutral one. At the beginning, I misunderstood that the usage of connectedRouterRedirect is only for redirecting not valid/logged-in users to the page for authentication, cuz authenticatedSelector makes me think that it implies a "valid" status. I guess the doc's small mistake corrected with this PR was likely caused by a moment of confusion on this too.

@mjrussell

1. Correct the code logic in the example of `userIsNotAuthenticated`.
2. Align minor inconsistencies in code examples.
@mjrussell
Copy link
Owner

Ah thanks, this is a good doc fix. Sorry its taken so long for me to reply. I agree the original name is a bit misleading, it probably should be called simply "shouldRedirectSelector" or something like that. I'm hesitant to make such a change because I'm not longer really actively maintaining this repo (#315) and would involve a lot of documentation updates & new major version.

@mjrussell mjrussell merged commit 6106f7c into mjrussell:master Mar 20, 2022
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

Successfully merging this pull request may close these issues.

2 participants