Refactor/1508 rewrite page navigation #1682
Merged
Conversation
This file contains 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
added 30 commits
November 15, 2023 11:18
…tering to recolor the logo
…tputting to use DisplayError.tsx. Committing this knowing that much of the code does not currently pass type-checks.
… putting what I can into a router
…rt problems with query providers
…t(), after implementing useAppReceiver()
…ontext/createLaxQueryContext into unified (and simplified + improved) variants
… ValidPartyProvider. There's still some inconsistencies around having a valid party. The InstantiationButton (for example) does not check to see if the current party is valid to instantiate before performing instantiation.
…ta is not yet fetched, mocking fetchTextResources() most places
…t, so that they work before app texts have been fetched
…ting runaway party validation
…o slow that this request halts, so the test should no longer assume it
# Conflicts: # src/components/altinnAppHeader.tsx # src/hooks/queries/useGetTextResourcesQuery.ts
…ge (this is most of the move, not all)
- Removing ValidPartyProvider.tsx, as validating the current party is done implicitly by checking if the party is in the list of valid parties we already fetch. We must have a valid party to proceed to instantiation. - Introducing a LanguageProvider that keeps tabs on which language is currently selected in the app. Loading a Profile will automatically select the language given in the profile. - Renaming reportee-selection.ts to party-selection.ts and adjusting the tests after minor (non-functional) changes
mikaelrss
commented
Dec 5, 2023
| @@ -15,12 +15,12 @@ export function mergeAndSort(...args: IAttachments[]) { | |||
| } | |||
|
|
|||
| // Sort all attachments by name | |||
| for (const nodeId in Object.keys(result)) { | |||
| Object.keys(result).forEach((nodeId) => { | |||
There was a problem hiding this comment.
This code produced a bug with the attachments-in-groups.ts cypress test. It iterated over the indices of the array instead of the keys. i.e [0,1,2,3] vs ["key-1", "key2"].
I guess it appeared because the state previousy was stored in a sorted order, and this now might not be the case. I am unsure why this has changed, but since the result always is sorted when we extract it, I don't see an issue here.
There was a problem hiding this comment.
It should probably have been a for ... of instead of a for ... in loop
mikaelrss
force-pushed
the
refactor/1508-rewrite-page-navigation
branch
from
8b5b090 to
53c1932
Compare
mikaelrss
requested review from
olemartinorg,
bjosttveit,
Magnusrm and
lassopicasso
mikaelrss
added
the
kind/breaking-change
…vider to renderWithInstanceAndLayout
|
SonarCloud Quality Gate failed.
|
This was referenced Oct 11, 2024
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
This PR refactors the current page navigation to use
react-router-dombased routing. Each step in the process is assigned a path in the url/:taskIdand each page of a step is assigned a path/:pageKey. For stateless apps, we assign only the/:pageKey.For pages on steps that are not the current process step, we allow the users to navigate here, but they will see a generic error message. The pages states that the current step is already completed, and provides a button for the user to navigate to the correct current step:
react-router-domrouter for page navigation.useNavigatePageanduseNavigationParams.PageNavigationContextwhich holds values that are related to component focus Id, scroll position, and hidden pages.UiConfigContextwhich holds values that were previously stored in redux. This highlighed a new problem. The current way of fetching these values uses the layout settings, which mean that for instanceshowLanguageSelectorcan only be specified in a layout set. This means that for instance selection and receipt pages, there is no way of showing the language selector. This worked previously because the settings of the last page was stored in redux and subsequently used in the receipt page. This is also why the Percy screenshot tests are failing.ProcessContext. Instead of storing the data fromtanstack/react-queryin auseStateand passing along thesetStatefunction to other components, I refactored this to instead update the query cache wherever the data was being updated. I discovered that this was necessary after unmountingProcessContextand remounting it, by navigating back from a form to instance selection and forward to the form again. Then all state about process was lost, and replaced by the cached query in tanstack, which thought the initial process request was the applicable state.// TODO(1508)comment. These tests need to be addressed in combination with Rewrite validation sagas to hooks/Tanstack query #1506 @bjosttveit.hideCloseButton,showLanguageSelector. Layout settings for receipt pages and instance selection #1686Related Issue(s)
Verification/QA
kind/*label to this PR for proper release notes grouping