Rework the app/progress indicator markup and styling #603
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
Refactored the React components and CSS components to do away with the Form layout component, favouring a single grid layout for the App container. This introduces more flexibility with the styling when re-arranging elements.
Rather than having the progress indicator component inherit the card component styles, it shares the implementation details with it but uses its own component namespace in the design tokens. Implemented in a backwards compatible manner to point to existing design tokens, but you can specify more specific ones if you'd like so.
* Use flexbox and gap for spacing between caption and link list in progress indicator * Hide the mobile header/toggle button on desktop * Manage spacing between list items with gap rather than margins
This now uses a flexbox approach for the content, while using the normal document flow as much as possible to fit the element inside its container element(s). The story is updated so that it's clear how overflowing content is handled. Padding is removed from the parent element and moved into the button element, so that it is more touch-device friendly. Tapping anywhere on the header now toggles it between open/closed state. Finally, most of these appearance aspects are now parametrized through design tokens.
When scrolling the main content, the progress indicator should stay in view.
This is quite involved as there are some styling edge cases to consider: * form title that overflows and is truncated with ellipsis - if you're not careful, this can cause a horizontal scrollbar * form title that is long and the main content being scrollable - this can also cause a small horizontal scrollbar due to the ellipsis situation * the progress indicator content itself should be scrollable, independent from the main content * possibly there is an external header/footer that may take up some vertical space, which limits the amount of vertical space available for our progress indicator The implementation now passes a ref to the button element in React, which allows us to calculate the height of the button (our 'menu' bar) AND its offset to the top of the page, in case there is another external UI element taking space. We then allocate a maximum block size (= vertical space) as the dynamic viewport block (100dvb, dynamic taking into account the address bar that can dissapear on actual mobile devices) minues the vertical/block spacing reserved for the button and possible third party content. When scrolling down, our button may overlay the third party content (due to the position: sticky) and this frees up a little bit of space below the progress indicator - this is deliberate. Users are allowed to interact with buttons below it and simplifies us having to track more things in React.
On mobile, apply some padding to push the language buttons down.
Would be really nice if storybook adds some before/after render hooks instead, so that we could do cleanup per story instead of on a global basis.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #603 +/- ##
==========================================
+ Coverage 71.39% 72.43% +1.04%
==========================================
Files 213 214 +1
Lines 4324 4335 +11
Branches 1155 1160 +5
==========================================
+ Hits 3087 3140 +53
+ Misses 1198 1159 -39
+ Partials 39 36 -3 ☔ View full report in Codecov by Sentry. |
The fixed steps being added don't need to be falsy, they need to not be added at all.
sergei-maertens
commented
Nov 27, 2023
The explicit z-index creates a stacking context which puts it on top of the date picker in appointments. Not specifying the z-index seems to work equally well.
sergei-maertens
commented
Nov 27, 2023
vaszig
reviewed
Nov 28, 2023
Sort-of - I fixed this by adding an additional design token, not sure why it isn't being picked up for you 🤔 |
Something went wrong with the previous version of design tokens, so I've redone them. Even the git reflog couldn't help :(
On request we can bring it back, but currently it doesn't seem that anyone makes use of this mechanism.
The overview should be navigable once all the preceding steps are completed, rather than always rendering a placeholder.
Fixed it with the latest commit! |
vaszig
approved these changes
Nov 29, 2023
|
The chromatic changes are accepted while they aren't intentional - there are some changes due to margins being applied now in combination with flexbox gaps, whereas before the bottom/top margins of different elements would collapse. There is also some styles being applied now in different order that cause the card body top margin to be 6px less. I'm not sure where exactly this is coming from, but that CSS is due a refactor anyway and should make use of design tokens rather than the current hardcoded values, so this will be revisited at some point. |
sergei-maertens
deleted the
refactor/36-progress-indicator-styling-rework
branch
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.
Closes #36
Depends on open-formulieren/design-tokens#45
Massive PR/refactor to redo the layout/markup of the root app element.
We now switch between grid layout for desktop/mobile so that we can apply the progress indicator stickiness in the appropriate
context, while ensuring that we can still properly scroll body content and scroll in the progress indicator if the height exceeds the available viewport height.
This is still all pretty complicated, but it should be more robust than what we had before.