Skip to content

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Nov 14, 2024

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

Copy link
Contributor

@mejiaj mejiaj left a comment

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

@mahoneycm mahoneycm requested a review from mejiaj November 20, 2024 21:54
@mahoneycm
Copy link
Contributor Author

@mejiaj Ready for re-review!

Copy link
Contributor

@amyleadem amyleadem left a comment

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.

@mandylloyd
Copy link
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>

Copy link
Contributor

@amyleadem amyleadem left a comment

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.

Copy link
Contributor

@amyleadem amyleadem left a comment

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

Copy link
Contributor

@thisisdano thisisdano left a comment

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
5 checks passed
@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