Fix page navigation when using returnUrl #2970
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bjosttveit
changed the title
|
adamhaeger
approved these changes
Feb 6, 2025
There was a problem hiding this comment.
Jeg er litt i tvil her. Funksjonaliteten er jo absolutt en forbedring, men det blir jo mye kompleks kode for å gjøre en relativt enkel ting.
Godkjenner PRen siden dette nok følger av strukturen i appen ellers, men viktig å se på arkitekturen rundt dette snart.
|
/publish |
PR release:
|
olemartinorg
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
The problem in the linked issue stemmed from how we figured out the next page when using
navigateToNextPage. It would look at the raw url and split on?and take the first part, assuming those only appear in the end of the hash part. But, when using a returnUrl, that is a "real" queryParameter before the hash even begins, so it would effectively remove all of the information in the hash and the next page could not be determined.I cleaned up the logic surrounding this, and also made some other changes. I tried to address the issues that caused us to add the hotfix where we disable the navigation buttons while saving. Instead, I now wait for saving after the button is clicked, showing a spinner on the one you clicked (next / previous) and disabling the other. Another issue I noticed when doing this is that the page order can change due to expressions after clicking the button now. Since we used to navigate directly to the page that was "next at the time of clicking the button", we would then potentially try to navigate to a page that was no longer visible. So I made some changes in
useNavigatePageso that we always use the "current" next page there, and now usenavigateToNextPagein the navigation buttons instead. I did a similar thing to the NavigationBarScreen.Recording.2025-02-05.at.11.53.40.mov
I then had some issues with
autoSaveBehavior: onChangePage. This used to be just "fire and forget", but now I wait until we don't have any unsaved changes here as well. Since this does not auto save, I had to make sure we keep requesting a manual save if there are new changes that need to be saved, or the waitForSave could get stuck since a new save does not get triggered automatically.Related Issue(s)
Verification/QA
kind/*label to this PR for proper release notes grouping