Persist the query params for migration flow to user navigation issue. #99443
+61
−8
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #98572
Relies on #99444.
Proposed Changes
Why are these changes being made?
As described in #98572, the context is currently lost and our user is exited in the wrong place.
This is because we don't pass along the
ref
properly as we navigate from view to view.I've decided to wrap the URLs with
persistQueryParams
which adds the current params to the new URL. When used withaddQueryArgs
, the parameters on the URL will be overwritten byaddQueryArgs
and therefore it's safe to do so.It does mean though that parameters we don't want to pass long will be passed which is why I haven't updated all references. I'm not certain that's expected.
Warning
Without an explicit state definition, I'm not certain I've updated all the relevant paths. It's a bit hard to read with the conditions.
Why not just pass the
ref
toaddQueryArgs
?To be honest, I was originally going to do this. I'm fine with this approach as well. I decided against it because I think it's ultimately safer to pass all the parameters based on how we are composing flows together. I can also see the other side as well that we should be explicitly setting this.
I can be swayed either way.
Why not wrap
navigate
oraddQueryArgs
?exitFlow
andnavigate
to direct users, we need to intercept earlier.addQueryArgs
is an external dependency and heavily used, wrapping that would be confusing.Is
persistQueryParams
the best function name?I don't know. :) Alternatives could be:
appendQueryParams
addQueryParamsToUrl
applyCurrentParams
Testing Instructions
Pre-merge Checklist
Note
I'll also add unit tests to
site-migration-flows.tsx
if this approach makes sense.