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

Enable back button functionality, clear url on load #91

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

WardBrian
Copy link
Collaborator

See discussion before merge in #84

This makes two changes:

  1. Query parameters are cleared on load, rather than on first edit. This "godbolt.org style" is my preference in the design space, but it also technically made item 2 much easier:
  2. Clicking the back button into a history item with the query parameters present leads to them actually loading in the UI

This also made it so the reducer no longer needs a callback to clear the URL on edit, since it will be done during load.

@magland
Copy link
Collaborator

magland commented Jun 26, 2024

I still prefer keeping the url intact as long as possible, and I'd like to propose a solution to this in the future. But because this is a simplification, I would be in favor of merging at this point.

@WardBrian WardBrian requested a review from jsoules June 27, 2024 19:06
Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

Approved, pending a recommended change to simplify the initial-load useEffect hook in the context provider.

@@ -38,11 +39,17 @@ const SPAnalysisContextProvider: FunctionComponent<PropsWithChildren<SPAnalysisC
}, [data])

useEffect(() => {
if (data != initialDataModel) return;
if (searchParams.size === 0 && data !== initialDataModel) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It occurs to me that this check might not be necessary if we remove the stan-playground-saved-state key from local storage regardless of which branch we take in the conditional.

Suppose searchParams is nontrivial. Then the first branch of the conditional below will trigger, ensuring that they'll be 0 on any subsequent loop.

Otherwise we'll check the status of the localStorage key. If that is set, we'll load it; if it is unset, no-op.

So, either we loaded the local storage (and don't need it any more), or we loaded the query string (and are actively ignoring the local storage values). Either way, the hook no longer needs to depend on the data member, which simplifies it further.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice spot!

@WardBrian WardBrian merged commit 5e41980 into main Jun 27, 2024
2 checks passed
@WardBrian WardBrian deleted the query-url-loading-back-button branch June 27, 2024 20:17
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.

3 participants