-
Notifications
You must be signed in to change notification settings - Fork 31
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
Moving ErrorReport component to the bottom of the form #346
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
…ally lots) of validation messages to pop up in the form above the button. Until now it's been slightly confusing when you end up scrolled to a position that doesn't make much sense, but not that really harmful. When the button is now being surrounded by a panel giving you a descriptive message detailing your mistakes, it's more important to show that message. Adding some state used by the NavigationButtons.tsx to scroll back into the same position (relative to the button) in case the page change is rejected. This should keep the button in view even if the actual form gets considerably longer in the Y direction.
…useState() combo with useMemo()
…x instead of on top of Presentation.tsx. Hopefully no other process step uses form validations. Instead of removing the bottom padding (as I did in v1), using negative a margin on FullWidthWrapper.tsx to consume it instead.
… and with the wrong list bullet)
…it so that it scrolls the correct component into view and attempts to focus on it. Removing usages in radio/checkboxes, as these had no particular effect earlier - but now they would scroll the component into view. Implementing clickable errors in ErrorReport.tsx that automatically sends you to the correct page and scrolls the component into view for you.
# Conflicts: # src/altinn-app-frontend/src/components/message/ErrorReport.tsx # src/altinn-app-frontend/src/components/presentation/NavigationButtons.tsx # src/altinn-app-frontend/src/features/form/containers/Form.tsx # src/altinn-app-frontend/src/shared/containers/Presentation.tsx
…eport.tsx and fixing minor issues there
…te these changes with my own
…the viewport (i.e. you don't scroll away from it when multiple validation errors appear above)
github-actions
bot
added
the
area/test
related to automated and functional testing
label
Jul 20, 2022
This was referenced Jul 20, 2022
…) so that TypeScript gives us an error in case a new variant gets added to the layout type string union without also being added to this switch/case.
…are above the viewport, and extending the test to verify that clicking the error message in ErrorReport scrolls you to the actual error message.
…urn behaviour of the Info panel type. Fixing this so the theoretically unreachable code can run when actual input doesn't adhere to TypeScript limitations.
haakemon
approved these changes
Jul 22, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! 🥳 Left some minor comments to consider.
src/altinn-app-frontend/src/features/form/containers/Form.test.tsx
Outdated
Show resolved
Hide resolved
src/altinn-app-frontend/src/features/form/components/FullWidthWrapper.tsx
Outdated
Show resolved
Hide resolved
src/altinn-app-frontend/src/features/form/components/FullWidthWrapper.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Håkon <[email protected]>
Co-authored-by: Håkon <[email protected]>
Co-authored-by: Håkon <[email protected]>
Co-authored-by: Håkon <[email protected]>
# Conflicts: # test/cypress/e2e/pageobjects/app-frontend.js
…to feat/100-errors-on-bottom-v2 # Conflicts: # test/cypress/e2e/pageobjects/app-frontend.js
…supposed to handle focus events such that you could trigger focus on the previous/next components in the layout (with the step argument). This breaks when dealing with repeating groups (ids with `myComponentId-0`), and it seems the code is not in use anymore - handleFocusUpdate() was declared in GenericComponent.tsx, but nobody ever uses it (anymore). Removing it entirely, as this should be re-implemented properly if needed. By using a simple reducer the updateFocus() action now works with componentIds pointing to a component in a repeating group.
…heckbox are already 'input')
mjulstein
reviewed
Jul 26, 2022
mjulstein
reviewed
Jul 26, 2022
mjulstein
reviewed
Jul 26, 2022
mjulstein
reviewed
Jul 26, 2022
…g tests in formLayout.test.ts (I recommend ignoring whitespace changes when reading this diff).
# Conflicts: # src/altinn-app-frontend/src/features/form/containers/Form.test.tsx # src/altinn-app-frontend/src/features/form/containers/Form.tsx # src/altinn-app-frontend/src/features/form/layout/update/updateFormLayoutSagas.test.ts
SonarCloud Quality Gate failed. |
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 change moved the
ErrorReport
component away from over the form modal to inside the modal, at the bottom of the form. Noteworthy changes:NavigationButtons
will now be incorporated into theErrorReport
should they be located at the end of the currently active layout when there are any validation errors.warning
variant, as theerror
variant is not implemented yet)next
navigation button, if validation messages are added to previous components in the form, you could end up seemingly being force-scrolled up a bit in the form (because added validation messages push the navigation buttons down). Now that validation messages are also displayed around these buttons, clicking the navigation buttons will maintain the position of the button(s) in the viewport when theErrorReport
shows up.ErrorReport
are clickable (and tab-able). Clicking them sends you to the component with the validation error (if any).Related Issue(s)
Verification
Documentation