[OF#3450] Fix styling issues of progress indicator #597
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #597 +/- ##
==========================================
+ Coverage 72.00% 72.27% +0.26%
==========================================
Files 213 213
Lines 4319 4324 +5
Branches 1152 1156 +4
==========================================
+ Hits 3110 3125 +15
+ Misses 1162 1160 -2
+ Partials 47 39 -8 ☔ View full report in Codecov by Sentry. |
…added aria-labelledby to nav
|
FYI: I wanted to approach the scrolling of the steps menu (mobile version) by targeting-selecting the parent class of the progress-indicator and add there the necessary rules but I read that this is not supported by all browsers, so I did this in a different way which I am not sure if it is correct. Another solution would be to refactor the whole progress indicator structure of the elements which I don't know if it is worth doing it. |
Remove padding on mobile story since we use the full viewport width, only apply the layout decorator to non-mobile stories.
There was a problem hiding this comment.
I'm trying out some things, discussed a bit on Slack as well and I need to time to investigate. The layout refactor caused a regression with the PI on desktop too - it is no longer (properly) sticky when scrolling down on large steps, but it also raises a concern for if there are many steps - how would that look and would it still be properly scrollable?
| @@ -201,3 +201,93 @@ export const ActiveSubmission = { | |||
| await userEvent.click(beginButton); | |||
| }, | |||
| }; | |||
|
|
|||
| export const SeveralStepsInMobileViewport = { | |||
There was a problem hiding this comment.
there are a bunch of prop type warnings in the console for this story, can you resolve those?
I also suspect some mocks are missing since I'm seeing network errors.
There was a problem hiding this comment.
The prop types are gone (fixed).
Concerning the mocks I don't get any errors only if I change the base url (like we have to do in order to see the results on a real phone). Is this what you mean?
There was a problem hiding this comment.
thanks! I think I will cherry pick some of your changes in what will become the final branch. I've learned a lot today and there are bits and pieces that I want to re-use, but ultimately I will throw away all existing CSS and re-build it to get rid of the cruft we've collected 😬
| @@ -36,15 +38,17 @@ const ProgressIndicator = ({ | |||
|
|
|||
| return ( | |||
| <Card blockClassName="progress-indicator" modifiers={modifiers}> | |||
| <nav> | |||
| <nav aria-labelledby="proress-indicator-section"> | |||
There was a problem hiding this comment.
typo in ID
| <nav aria-labelledby="proress-indicator-section"> | |
| <nav aria-labelledby="progress-indicator-section"> |
additionally you want to namespace IDs/have them be unique since the ID could already be present when embedding in a CMS page.
Perhaps using a UUID is more robust for this.
There was a problem hiding this comment.
Do you mean to generate a UUID here?
| .openforms-theme { | ||
| .utrecht-link { | ||
| @include bem.modifier('openforms-active') { | ||
| font-weight: bold; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
why not apply utrecht-link--current classname? There is a design token for this: --utrecht-link-current-font-weight.
| @include bem.modifier('mobile-expanded') { | ||
| top: 0; | ||
| left: 0; | ||
| position: fixed; | ||
| width: 100%; | ||
| height: 100%; | ||
| overflow-y: auto; | ||
| } |
There was a problem hiding this comment.
Couple problems - I've added a dummy nav element that could/would be present on a CMS page where the SDK is being embedded. You can assume that this will also have mobile/responsive menu where it is "glued" to the top of the page and that this nav should be available at all times:
HTML:
When I open the PI, it overlays the entire page:
I'm not sure if this is desirable or not 🤔 it's easy enough to dismiss it again, but it also a bit weird that it takes over the entire screen?
There was a problem hiding this comment.
In my opinion, it depends on how we want it to behave. If this should be a simple dropdown the approach is wrong. If we want it to be like a button which opens a popup then it's kind of correct, and I am saying kind of because what it really does is hiding the background. If someone changes the position the begin button could be clicked.
There was a problem hiding this comment.
I've done some sketching on paper on what the desired behaviour is, I'll make sure to document that on the components so that it's visible in storybook documentation. It's an overlay that expands to the bottom - it does not necessarily need to cover the entire screen (but it's also fine if it does). It needs to play well with headers and footers above/below the form though.
|
Replaced by #603 |
sergei-maertens
deleted the
task/2139-3450-fix-progress-indicator-styling
branch
Fixes open-formulieren/open-forms#3450
Fixes open-formulieren/open-forms#2139
Fixes #36
langattribute, so in our situation the browser will detect the language. The browser is free to automatically break words at appropriate hyphenation points when we definehyphens: autonavelements in a single page. I have addedaria-labelledbyto thenavin case we embed the form (OpenInwoner already has anav).