Skip to content
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.

Sign up for GitHub

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

[OF#3450] Fix styling issues of progress indicator #597

Closed
wants to merge 3 commits into from
Closed

[OF#3450] Fix styling issues of progress indicator #597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a565209
[open-formulieren/open-forms#3450] Fixed wordbreak and scoped css for…
vaszig Nov 21, 2023
b6895ec
[open-formulieren/open-forms#3450] Fixed scrolling in mobile version,…
vaszig Nov 22, 2023
795e216
:pencil: Tweak stories layout for more representative padding/background
sergei-maertens Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 114 additions & 4 deletions src/components/App.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {SUBMISSION_ALLOWED} from './constants';
export default {
title: 'Private API / App',
component: App,
decorators: [LayoutDecorator, ConfigDecorator],
decorators: [ConfigDecorator],
args: {
'form.translationEnabled': true,
submissionAllowed: SUBMISSION_ALLOWED.yes,
Expand Down Expand Up @@ -113,10 +113,11 @@ const render = args => {

export const Default = {
render,
decorators: [LayoutDecorator],
};

export const TranslationEnabled = {
render,
...Default,
args: {
'form.translationEnabled': true,
},
Expand All @@ -127,7 +128,7 @@ export const TranslationEnabled = {
};

export const TranslationDisabled = {
render,
...Default,
args: {
'form.translationEnabled': false,
},
Expand All @@ -145,8 +146,8 @@ export const TranslationDisabled = {
};

export const ActiveSubmission = {
...Default,
name: 'Active submission',
render,
decorators: [
// remove the window.localStorage entry, UUID value is from `api-mocks/forms.js`.
// it gets set because of the play function which starts a submission.
Expand Down Expand Up @@ -201,3 +202,112 @@ export const ActiveSubmission = {
await userEvent.click(beginButton);
},
};

export const SeveralStepsInMobileViewport = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😬

render,
args: {
title: 'Progress',
formTitle: 'Formulier',
steps: [
vaszig marked this conversation as resolved.
Show resolved Hide resolved
{
uuid: '9e6eb3c5-e5a4-4abf-b64a-73d3243f2bf5',
slug: 'step-1',
formDefinition: 'Step 1',
index: 0,
vaszig marked this conversation as resolved.
Show resolved Hide resolved
url: `${BASE_URL}forms/mock/steps/9e6eb3c5-e5a4-4abf-b64a-73d3243f2bf5`,
isApplicable: true,
completed: false,
},
{
uuid: '71fa50c1-53db-4179-9fe7-eb3378ef39ee',
slug: 'step-2',
formDefinition: 'Step 2',
index: 0,
url: `${BASE_URL}forms/mock/steps/71fa50c1-53db-4179-9fe7-eb3378ef39ee`,
isApplicable: true,
completed: false,
},
{
uuid: 'f4f82113-ac83-429b-a17b-c1b145831fa9',
slug: 'step-3',
formDefinition: 'Step 3',
index: 0,
url: `${BASE_URL}forms/mock/steps/f4f82113-ac83-429b-a17b-c1b145831fa9`,
isApplicable: true,
completed: false,
},
{
uuid: 'ae5af44e-004f-4d2f-be75-d46a22577244',
slug: 'step-4',
formDefinition: 'Step 4',
index: 0,
url: `${BASE_URL}forms/mock/steps/ae5af44e-004f-4d2f-be75-d46a22577244`,
isApplicable: true,
completed: false,
},
{
uuid: 'cae11cf8-2773-4d70-80f4-71e5828b6fe3',
slug: 'step-5',
formDefinition: 'Step 5',
index: 0,
url: `${BASE_URL}forms/mock/steps/cae11cf8-2773-4d70-80f4-71e5828b6fe3`,
isApplicable: true,
completed: false,
},
{
uuid: '3b14f4f5-2283-4a81-adf1-03848672c83b',
slug: 'step-6',
formDefinition: 'Step 6',
index: 0,
url: `${BASE_URL}forms/mock/steps/3b14f4f5-2283-4a81-adf1-03848672c83b`,
isApplicable: true,
completed: false,
},
{
uuid: '3f0a2763-74de-4957-b970-a5117f15b023',
slug: 'step-7',
formDefinition: 'Step 7',
index: 0,
url: `${BASE_URL}forms/mock/steps/3f0a2763-74de-4957-b970-a5117f15b023`,
isApplicable: true,
completed: false,
},
{
uuid: '03657dc1-6bb1-49cf-8113-4b663981b70f',
slug: 'step-8',
formDefinition: 'Step 8',
index: 0,
url: `${BASE_URL}forms/mock/steps/03657dc1-6bb1-49cf-8113-4b663981b70f`,
isApplicable: true,
completed: false,
},
{
uuid: '03657dc1-6bb1-49cf-8113-4b663981b70f',
slug: 'step-9',
formDefinition: 'Step 9',
index: 0,
url: `${BASE_URL}forms/mock/steps/03657dc1-6bb1-49cf-8113-4b663981b70f`,
isApplicable: true,
completed: false,
},
{
uuid: '03657dc1-6bb1-49cf-8113-4b663981b70f',
slug: 'step-10',
formDefinition: 'Step 10',
index: 0,
url: `${BASE_URL}forms/mock/steps/03657dc1-6bb1-49cf-8113-4b663981b70f`,
isApplicable: true,
completed: false,
},
],
ariaMobileIconLabel: 'Progress step indicator toggle icon (mobile)',
accessibleToggleStepsLabel: 'Current step in form Formulier: Stap 2',
},
parameters: {
chromatic: {disableSnapshot: true},
layout: 'fullscreen',
viewport: {
defaultViewport: 'mobile1',
},
},
};
8 changes: 6 additions & 2 deletions src/components/Caption.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@ import React from 'react';

import {getBEMClassName} from 'utils';

const Caption = ({children, component = 'caption'}) => {
const Caption = ({children, id, component = 'caption'}) => {
vaszig marked this conversation as resolved.
Show resolved Hide resolved
const Component = `${component}`;
return <Component className={getBEMClassName('caption')}>{children}</Component>;
return (
<Component id={id} className={getBEMClassName('caption')}>
{children}
</Component>
);
};

Caption.propTypes = {
Expand Down
10 changes: 7 additions & 3 deletions src/components/ProgressIndicator/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
const [expanded, setExpanded] = useState(false);

const modifiers = [];
if (!expanded) {
if (expanded) {
modifiers.push('mobile-expanded');

Check warning on line 24 in src/components/ProgressIndicator/index.js

View check run for this annotation

Codecov / codecov/patch

src/components/ProgressIndicator/index.js#L24 

Added line #L24 was not covered by tests
} else {
modifiers.push('mobile-collapsed');
}

Expand All @@ -36,15 +38,17 @@

return (
<Card blockClassName="progress-indicator" modifiers={modifiers}>
<nav>
<nav aria-labelledby="proress-indicator-section">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in ID

Suggested change
<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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to generate a UUID here?

<MobileButton
ariaMobileIconLabel={ariaMobileIconLabel}
accessibleToggleStepsLabel={accessibleToggleStepsLabel}
formTitle={formTitle}
expanded={expanded}
onExpandClick={() => setExpanded(!expanded)}
/>
<Caption component="h2">{title}</Caption>
<Caption id="proress-indicator-section" component="h2">
{title}
</Caption>
<List ordered>
{steps.map((step, index) => (
<ProgressIndicatorItem
Expand Down
12 changes: 8 additions & 4 deletions src/scss/components/_anchor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@
}
}

@include bem.modifier('openforms-active') {
font-weight: bold;
}

@include bem.modifier('openforms-inherit') {
color: inherit;

Expand Down Expand Up @@ -72,3 +68,11 @@
@extend .utrecht-link, .utrecht-link--openforms;
}
}

.openforms-theme {
.utrecht-link {
@include bem.modifier('openforms-active') {
font-weight: bold;
}
}
}
Comment on lines +72 to +78
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not apply utrecht-link--current classname? There is a design token for this: --utrecht-link-current-font-weight.

4 changes: 4 additions & 0 deletions src/scss/components/_app.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@
display: flex;
}

@include bem.element('body') {
flex: 1;
}
vaszig marked this conversation as resolved.
Show resolved Hide resolved

@include bem.element('debug') {
margin-block-start: 2em;
}
Expand Down
11 changes: 10 additions & 1 deletion src/scss/components/_progress-indicator.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,15 @@
display: none;
}
}

@include bem.modifier('mobile-expanded') {
top: 0;
left: 0;
position: fixed;
width: 100%;
height: 100%;
overflow-y: auto;
}
Comment on lines +84 to +91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

HTML:

image

When I open the PI, it overlays the entire page:

image

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand All @@ -94,6 +103,6 @@

@include bem.element('label') {
flex-grow: 1;
word-break: break-all;
hyphens: auto;
}
}