-
Notifications
You must be signed in to change notification settings - Fork 1.1k
USWDS - Storybook: Add margin to form error test preview #6204
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.
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
Conversation
There was a problem hiding this 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.
@mejiaj Ready for re-review! |
There was a problem hiding this 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.
@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 |
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple and effective
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
Testing and review
error_state
istrue
, confirm you can see left border error