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

1321 move routing logic to its own file so that it is a child compone… #1337

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

schroerbrian
Copy link
Contributor

…nt of the AppProvider component and will thus have access to the auth state

This is a preparatory PR that just moves all of the routing logic into a new component that is a child of the AppContext and thus has access to the AppContext's state props. There are no other changes. My subsequent Navigator Dashboard PR (coming soon), will contain a couple changes to the routes file. I didn't want to include what are essentially copy and paste changes in that PR for ease of reviewing.

Copy link
Member

@richardxia richardxia left a comment

Choose a reason for hiding this comment

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

This generally looks good to me, but super nit: I feel like the routes is super important to a React app, more so than being just a "utility component", so maybe we should even just have it directly in the app/ directory? I think our app used to actually have a file at app/routes.jsx with the routes, but we had merged it into App.jsx at some point. There's no one standard React app layout, but by analogy in Rails and Django, you have pretty prominent files at config/routes.rb and urls.py that hold the main app routing logic, neither of which are "utils".

@schroerbrian
Copy link
Contributor Author

This generally looks good to me, but super nit: I feel like the routes is super important to a React app, more so than being just a "utility component", so maybe we should even just have it directly in the app/ directory? I think our app used to actually have a file at app/routes.jsx with the routes, but we had merged it into App.jsx at some point. There's no one standard React app layout, but by analogy in Rails and Django, you have pretty prominent files at config/routes.rb and urls.py that hold the main app routing logic, neither of which are "utils".

Thanks! That makes total sense. I have seen that pattern before.

@schroerbrian
Copy link
Contributor Author

Note: i'm keeping the empty components/utils directory as i'll be placing another file in that directory in an imminent PR

@schroerbrian schroerbrian merged commit f03a01a into master Apr 12, 2024
5 checks passed
@schroerbrian schroerbrian deleted the 1321-create-router-file branch April 12, 2024 22:00
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