Skip to content

USWDS - Storybook: Add margin to form error test preview#6204

Merged
thisisdano merged 4 commits into
developfrom
cm-error-story-margin
Dec 16, 2024
Merged

USWDS - Storybook: Add margin to form error test preview#6204
thisisdano merged 4 commits into
developfrom
cm-error-story-margin

Conversation

@mahoneycm

@mahoneycm mahoneycm commented Nov 14, 2024

Copy link
Copy Markdown
Contributor

Summary

Improved the display of form error states in desktop views by adding margin to the form element. Now, error borders are viewable at all widths in the preview.

This is a lighthanded alternative approach from #5999

Breaking change

This is not a breaking change

Related issue

Closes #5998

Related pull requests

#5999

Preview link

Form error test story →

Problem statement

Error states use a negative margin in desktop widths. When previewed in storybook, they are generally on their own and thus, do not have enough margin to preview error states.

Solution

Add margin to error state preview using utility class to properly preview error states.

Alternative solutions considered

This approach only affects the larger developer preview for error states. We are currently not offering individual error state previews per component but if we do, we should find another approach so that utility classes do not make their way into component code previews.

Major changes

  • Error state borders are now viewable in test story at desktop widths.

Testing and review

  1. Visit Test error form elements storybook preview
  2. When error_state is true, confirm you can see left border error

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mahoneycm thanks for this work. Re-scoping it to this issue makes the review much easier!

One thing I noticed is that the templates are rendering what seems like duplicates multiple times, is this intentional? Screenshot attached.

CleanShot 2024-11-18 at 11 51 38@2x

Comment thread packages/usa-form/src/test/test-pattern/test-usa-form.twig Outdated
Comment thread packages/usa-form/src/test/test-pattern/test-usa-form.twig
Comment thread packages/usa-form/src/test/test-pattern/test-usa-form.twig Outdated
@mahoneycm mahoneycm requested a review from mejiaj November 20, 2024 21:54
@mahoneycm

Copy link
Copy Markdown
Contributor Author

@mejiaj Ready for re-review!

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this @mahoneycm.

I had a question about scope for this change. Is it possible to extend this solution to the components that have built in error states or error stories? I think it just includes text input, file input, and character count (but it is worth double checking).

I was also curious if we could target the horizontal padding only.

Comment thread packages/usa-form/src/usa-form.stories.js Outdated
@mandylloyd

Copy link
Copy Markdown
Contributor

@mahoneycm not sure if it would be out of scope for this task but since I've got RTL styling always top of mine I took a look and noticed this border does not mirror correctly when the language direction is switched. Should be a quick easy lift if it's within scope! You can test by dir="rtl" to <html>

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One more small change request. Not a blocker, but would make mobile testing a bit easier for this story.

Comment thread packages/usa-form/src/usa-form.stories.js

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @mahoneycm. I like the simple solution here and look forward to continuing the work in #6240

@thisisdano thisisdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simple and effective

@thisisdano thisisdano merged commit f041c94 into develop Dec 16, 2024
@thisisdano thisisdano deleted the cm-error-story-margin branch December 16, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

USWDS - Forms: Form group error left border only viewable on wide widths

5 participants