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

Use stanard React Router over custom useRoute #81

Closed
jsoules opened this issue Jun 25, 2024 · 5 comments · Fixed by #83
Closed

Use stanard React Router over custom useRoute #81

jsoules opened this issue Jun 25, 2024 · 5 comments · Fixed by #83

Comments

@jsoules
Copy link
Collaborator

jsoules commented Jun 25, 2024

I thought I had an issue for this already, but it looks like not, I guess we only discussed in person.

In reviewing PR #65 as part of the discussion around PR #77, I realized how much the useRoute infrastructure will need to grow to support query parsing.

I think we will be much better off moving away from custom routing logic implementation and using stock React Router functionality (both the use-search-params hook, as well as more standard route structuring); & I think this should be a part of the query parameter support PR. I have a model for this in the QUASR Navigator at https://github.com/jsoules/stellarator-navigator/blob/trunk/stellarator-navigator/src/main.tsx and happy to help with it.

@magland
Copy link
Collaborator

magland commented Jun 25, 2024

Sounds good, but before fussing with the implementation, I think it would be helpful to make a design spec for exactly what the url query parameters should look like.

Would it deviate from this?
#65 (comment)

@WardBrian
Copy link
Collaborator

I don’t think the URL structure will differ from what we had previously discussed there

@jsoules
Copy link
Collaborator Author

jsoules commented Jun 25, 2024

Sounds good, but before fussing with the implementation, I think it would be helpful to make a design spec for exactly what the url query parameters should look like.

Would it deviate from this? #65 (comment)

I agree that that URL structure is fine, though I'd also be happy to allow both sampling_opts and inline_opts with a last-one-wins (or query string beats other source). No need to worry over that right now--should be easy enough to change it later if we want.

@WardBrian WardBrian linked a pull request Jun 26, 2024 that will close this issue
@magland
Copy link
Collaborator

magland commented Jun 26, 2024

@WardBrian Why closed? Haven't implemented new router logic yet.

@WardBrian
Copy link
Collaborator

Issue #50 seems to cover the re-implementation better

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 a pull request may close this issue.

3 participants