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.

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

USWDS - Storybook: Create form error story #5879

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Apr 22, 2024

Summary

Added form input error state showcase and controls to forms pattern. This is a general display for development testing and reference.

There are a few issues we might want to address if we want to normalize form input error states across the design system. I've created #5884 to track the issues.

Note

I've also created this draft PR that includes this work and brings error state stories to each individual component story page.

Note that this work might be slightly out of date with the final error showcase PR found in this PR.

Breaking change

This is not a breaking change.

Related issue

Closes #5839

Preview link

Error Form Elements →

Problem statement

Currently, there is no preview or testing environment for form input error states.

Solution

Create a story to showcase form elements in error states. This new story has a boolean error_state control that developers can use to toggle the error state on and off.

When in error state, the twig file conditionally adds:

  • usa-form-group & usa-form-group--error classes where needed
    • In some cases, a new div is created to wrap form content
  • usa-label—error and “…has an error” labels
  • usa-error-message spans to showcase helpful error messages
  • usa-input--error to form elements with eligible input elements

Major changes

New Form pattern: Error Form Element

Testing and review

  1. Visit Form error state pattern story
  2. Confirm all forms have an error state
    • Note that some components such as combo box does not have error styles on their inputs due to initialization bug
  3. Confirm error state elements have the red left border when view is lower than desktop widths
  4. Confirm error state control works and is named appropriately
  5. Inspect component markup and logic based DOM structure

Important

Due to initialization bugs, Combo box, input mask, and Time Picker are broken with the error state. Additional information is available in comments in this thread.

Copy link
Contributor Author

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Additional information and findings

Comment on lines 20 to 26
<div class="usa-form-group {% if error_state %}usa-form-group--error{% endif %}">
<fieldset class="usa-fieldset margin-top-2">
<legend class="usa-legend {% if error_state %}usa-label--error{% endif %}">Checkbox{% if error_state %} has an error{% endif %}</legend>
{% if error_state %}<span class="usa-error-message" role="alert">Helpful error message</span>{% endif %}
<div class="usa-checkbox">
<input
class="usa-checkbox__input usa-input--error"
Copy link
Contributor Author

@mahoneycm mahoneycm Apr 24, 2024

Choose a reason for hiding this comment

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

Combo box loses input error class on init

Warning

Adding the usa-input--error class to the combo box nested select element does not carry the class over to its dynamically placed input element. The class remains on the hidden select element. This is true for time picker's combo box element as well.

For situations like these, it may be best to carry over classes from the placeholder element, similar to how we carry over attributes with Modal

It is possible to add the classes to the input element after the component is initialized and this may be the case for most form error validation but I think it's important to make sure it's able to initialize in this state in the event a form causes the page to refresh.

Comment on lines 162 to 165
{% if error_state %}<div class="usa-form-group usa-form-group--error">{% endif %}
<label class="usa-label {% if error_state %}usa-label--error{% endif %}" for="example-input-prefix">Input prefix{% if error_state %} has an error{% endif %}</label>
{% if error_state %}<span class="usa-error-message" role="alert">Helpful error message</span>{% endif %}
<div class="usa-input-group {% if error_state %}usa-input--error{% endif %}">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Input prefix/suffix input group

Tip

We must put the usa-input--error class on the usa-input-group element in order for the styles to display as expected

<legend class="usa-legend">Checkbox</legend usa-label>
<div class="usa-form-group {% if error_state %}usa-form-group--error{% endif %}">
<fieldset class="usa-fieldset margin-top-2">
<legend class="usa-legend {% if error_state %}usa-label--error{% endif %}">Checkbox{% if error_state %} has an error{% endif %}</legend>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Legend and error labels

Tip

usa-legend and usa-label output the same base styles. Because of this, I was able to safely add the usa-label--error class without any additional changes or unexpected style conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an opinionated choice and might cause confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively we could add the usa-label class as well but this does not introduce any styles that are not already present thanks to usa-legend

Semi-related, we might want to move the duplicate styles from both definitions to a mixin to reduce duplicate code:

.usa-legend {
@include typeset(
$theme-form-font-family,
$theme-body-font-size,
$theme-input-line-height
);
display: block;
font-weight: font-weight("normal");
margin-top: units(3);
max-width: units($theme-input-max-width);
}

.usa-label {
@include typeset(
$theme-form-font-family,
$theme-body-font-size,
$theme-input-line-height
);
display: block;
font-weight: font-weight("normal");
margin-top: units(3);
max-width: units($theme-input-max-width);
}

I've created #5894 for additional discussion and to track work if we decide to move forward with this refactor

Comment on lines 1 to +3
<form class="usa-form">
<div class="usa-character-count">
<div class="usa-form-group">
<label class="usa-label" for="with-hint-input">Character count</label>
<div class="usa-form-group {% if error_state %}usa-form-group--error{% endif %}">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

usa-form and usa-form-group

Note

While we could add the usa-form-group--error class to <form class="usa-form"> elements, I believe we should avoid doing this and only add it to usa-form-group elements.

Setting the error on the form element would add the error class to the entire form and not just the form question that holds the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a useful comment in code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added in 1723270

Comment on lines 143 to 144
{% if error_state %}<div class="usa-form-group usa-form-group--error">{% endif %}
<label class="usa-label {% if error_state %}usa-label--error{% endif %}" for="ssn">Input mask{% if error_state %} has an error{% endif %}</label>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Input mask error

Warning

Input mask breaks due to DOM structure changing.

Related to: #5517

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also noting this error is causing the time picker to not initialize properly in the storybook preview

Copy link
Contributor

Choose a reason for hiding this comment

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

Time picker doesn't initialize because of the new wrapper? Same issue as input mask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Time picker doesn't initialize because of the JS error caused by input mask. Removing input mask or the usa-form-group wrapper will allow the time picker to initialize correctly.

I added a comment in 1723270 to help outline this issue.

Comment on lines 161 to +162
<form class="usa-form">
<label class="usa-label" for="example-input-suffix">Input suffix</label>
<div class="usa-input-group usa-input-group--sm">
<input
id="example-input-suffix"
class="usa-input"
pattern="[0-9]*"
inputmode="numeric"
{% if disabled_state == 'disabled' %} disabled
{% elseif disabled_state == 'aria-disabled' %} aria-disabled="true" {%- endif %}
/>
<div class="usa-input-suffix" aria-hidden="true">lbs.</div>
</div>
</form>

<fieldset class="usa-fieldset">
<legend class="usa-legend">Memorable date</legend>
<span class="usa-hint" id="mdHint">For example: January 19 2000</span>
<div class="usa-memorable-date">
<div class="usa-form-group usa-form-group--month usa-form-group--select">
<label class="usa-label" for="date_of_birth_month">Month</label>
<select
class="usa-select"
id="date_of_birth_month"
name="date_of_birth_month"
aria-describedby="mdHint"
{% if disabled_state == 'disabled' %} disabled
{% elseif disabled_state == 'aria-disabled' %} aria-disabled="true" {%- endif %}
>
<option value>- Select -</option>
<option value="1">01 - January</option>
<option value="2">02 - February</option>
<option value="3">03 - March</option>
<option value="4">04 - April</option>
<option value="5">05 - May</option>
<option value="6">06 - June</option>
<option value="7">07 - July</option>
<option value="8">08 - August</option>
<option value="9">09 - September</option>
<option value="10">10 - October</option>
<option value="11">11 - November</option>
<option value="12">12 - December</option>
</select>
</div>
<div class="usa-form-group usa-form-group--day">
<label class="usa-label" for="date_of_birth_day">Day</label>
{% if error_state %}<div class="usa-form-group usa-form-group--error">{% endif %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding usa-form-group divs

Note

In order for the red border to highlight the form input with an error, the elements must be nested within a div with the usa-form-group and usa-form-group--error classes.

We should consider the implications of this while creating guidance for setting error states.

I've used if statements to wrap the opening and closing tags to only add the divs when the error state is active.

packages/usa-form/src/test/test-pattern/test-usa-form.twig Outdated Show resolved Hide resolved
>Time picker</label
<div class="usa-form-group {% if error_state %}usa-form-group--error{% endif %}">
<label class="usa-label {% if error_state %}usa-label--error{% endif %}" id="appointment-time-label" for="appointment-time"
>Time picker{% if error_state %} has an error{% endif %}</label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Time picker error input

Warning

Due to Combo box's dynamic input element not receiving the same classes as it's select placeholder, the error input classes are not set on initialization

@mahoneycm mahoneycm marked this pull request as ready for review April 25, 2024 13:53
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.

The warnings and other notes you've set in this PR are helpful. It'd be nice to add them to the code (preferably if they're linked to an issue) so that it doesn't get lost.

Something for us to keep in mind moving forward, but we should move away from hardcoding component markup as much as possible. This isn't re-usable and can introduce bloat into the codebase.

I understand the intent of this PR is to see the current state of errors in USWDS forms. To accomplish that we should show the most accurate state, based on existing guidance, and not try to improvise fixes or solutions.


Can you add comments in code where necessary and remove any workarounds?

<div class="usa-form-group">
<label class="usa-label" for="with-hint-input">Character count</label>
<div class="usa-form-group {% if error_state %}usa-form-group--error{% endif %}">
<label class="usa-label {% if error_state %}usa-label--error{% endif %}" for="with-hint-input">Character count{% if error_state %} has an error{% endif %}</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

The spacing for has an error looks inconsistent.

Comment on lines 143 to 144
{% if error_state %}<div class="usa-form-group usa-form-group--error">{% endif %}
<label class="usa-label {% if error_state %}usa-label--error{% endif %}" for="ssn">Input mask{% if error_state %} has an error{% endif %}</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Time picker doesn't initialize because of the new wrapper? Same issue as input mask?

Comment on lines 1 to +3
<form class="usa-form">
<div class="usa-character-count">
<div class="usa-form-group">
<label class="usa-label" for="with-hint-input">Character count</label>
<div class="usa-form-group {% if error_state %}usa-form-group--error{% endif %}">
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a useful comment in code.

packages/usa-form/src/test/test-pattern/test-usa-form.twig Outdated Show resolved Hide resolved
<legend class="usa-legend">Checkbox</legend usa-label>
<div class="usa-form-group {% if error_state %}usa-form-group--error{% endif %}">
<fieldset class="usa-fieldset margin-top-2">
<legend class="usa-legend {% if error_state %}usa-label--error{% endif %}">Checkbox{% if error_state %} has an error{% endif %}</legend>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an opinionated choice and might cause confusion.

@mahoneycm
Copy link
Contributor Author

mahoneycm commented Apr 26, 2024

@mejiaj I've added my GH comments as liquid twig syntax comments on the twig template for more visibility. I've linked #5884 to discuss additional work and improvements needed for a uniform experience across form inputs

@mahoneycm mahoneycm requested a review from mejiaj April 26, 2024 16:57
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.

Looks good, didn't find any a11y issues with axe devtools either.

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 - Feature Request: Create usa-form-group--error story
2 participants