Skip to content

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented May 1, 2024

Summary

Enhanced visual cue when maxlength is exceeded. Now, the component uses standard USWDS error styles to visually enhance the error state.

Breaking change

This is not a breaking change.

Related issue

Closes #5851

Related pull requests

Changelog →

Preview link

New visual change only

This link contains only the changes from this PR
Character count →

Combined with additional styles

This preview combines this work with #5999. This is what the change will look like on release.
Character count preview with enhanced form group error styles →

Link to demo branch

Problem statement

Testing results show that users expect additional indication that they have exceeded the limit. Additionally, screen magnification users may be unable to see the character count status message beneath the textarea input.

Solution

Add additional visual cues to let users know they have exceeded the character count limit.

Major changes

  • When users surpass the character limit, the relevant error classes are added to the form input, group, and label elements
  • This class will be removed when the character count is beneath the limit

Testing and review

Character count behaviors

  1. Visit the demo branch preview →.
  2. For both input variants, surpass relevant character limits.
  3. Confirm label, input, and form group error classes and styles are applied correctly.
  4. Backspace input to validate character count.
  5. Confirm error class and styles are removed.
  6. Run unit test and confirm there are no errors.

Testing checklist

  • npm run test runs without error
  • usa-input--errorclasses are applied on character input over character limit
  • usa-input--errorclasses are removed on character input under character limit
  • usa-form-group--error classes are applied on character input over character limit
  • usa-form-group--error classes are removed on character input under character limit
  • usa-label--error classes are applied on character input over character limit
  • usa-label--error classes are removed on character input under character limit
  • Character count JS matches USWDS code standards.
  • Unit tests match USWDS code standards // Unit tests removed

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.

Due to the unit test updates, there are a handful of changed files with this PR. Added some comments to help add clarity to each of the changes:

Comment on lines 38 to 49
const formGroupEl = characterCountEl.querySelector(FORM_GROUP);

if (!formGroupEl) {
throw new Error(`${CHARACTER_COUNT} is missing inner ${FORM_GROUP}`);
}

const labelEl = characterCountEl.querySelector(LABEL);

if (!labelEl) {
throw new Error(`${CHARACTER_COUNT} is missing inner ${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.

Note

Added checks for form group and label elements. These elements are necessary for the overall structure of the component and will result in JS errors for toggleErrorState if they are missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed errors in fe42118

Comment on lines 8 to 13
const FORM_GROUP_CLASS = `${PREFIX}-form-group`;
const FORM_GROUP_ERROR_CLASS = `${FORM_GROUP_CLASS}--error`;
const FORM_GROUP = `.${FORM_GROUP_CLASS}`;
const LABEL_CLASS = `${PREFIX}-label`;
const LABEL_ERROR_CLASS = `${LABEL_CLASS}--error`;
const LABEL = `.${LABEL_CLASS}`;
Copy link
Contributor Author

@mahoneycm mahoneycm May 2, 2024

Choose a reason for hiding this comment

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

Note

Added classes and elements used to add error states. Organized by when they appear in markup/DOM

Comment on lines +226 to +228
FORM_GROUP_ERROR_CLASS,
LABEL_ERROR_CLASS,
INPUT_ERROR_CLASS,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

Exported for use in new unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated unit test templates

I added usa-form-group and usa-label's to all character count unit tests for consistency and to prevent JS errors now that the component checks for these elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

New unit test to check for the existence of form group element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: Remove unit test if we remove related 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.

Note

New unit test to check for the existence of the label element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: Remove unit test if we remove related errors

@@ -150,9 +169,25 @@ const updateCountMessage = (inputEl) => {
inputEl.setCustomValidity("");
}

inputEl.classList.toggle(INPUT_ERROR_CLASS, isOverLimit);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

Adds / removes input error classes to match invalid status message class

@mahoneycm mahoneycm requested review from mejiaj, amyleadem, jaclinec and amycole501 and removed request for amycole501 May 2, 2024 15:24
@mahoneycm mahoneycm marked this pull request as ready for review May 2, 2024 15:24
@amycole501
Copy link

@mahoneycm we think there might be an issue with the blue focus indicator and the red indicator not having enough contrast with one another. Is there a way to add a small space between the two (maybe a few pixels) to make a distinction there are two states happening (one focus, one error)? https://www.w3.org/WAI/WCAG22/Understanding/focus-appearance.html

@mahoneycm
Copy link
Contributor Author

@amycole501 Here's what it looks like it with an offset of 2px

image
Kapture.2024-05-03.at.15.59.41.webm

The input shrinks a bit but this is actually a result of the error border being added and not the outline offset. The same difference in input size occurs with or without the offset.

What do we think about something like this?

This issue might be a bit out of scope for this specific issue but definitely something we should consider with #5884

CC: @jaclinec @mejiaj @amyleadem

@amycole501
Copy link

I like it! Would like to know what @alex-hull thinks but the separation makes it much easier to see the two borders.

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.

@mahoneycm
I started testing the interactions on this and generally it looks good! I had one question below about what happens after you fix an error (shown in the comment below). Let me know if you want to discuss!

I'll return to the code review once we figure that out.

@mahoneycm mahoneycm requested a review from amyleadem May 23, 2024 14:06
@mahoneycm
Copy link
Contributor Author

mahoneycm commented Jul 1, 2024

@amycole501 Here's what it looks like it with an offset of 2px

image The input shrinks a bit but this is actually a result of the error border being added and not the outline offset. The same difference in input size occurs with or without the offset.

What do we think about something like this?

This issue might be a bit out of scope for this specific issue but definitely something we should consider with #5884

CC: @jaclinec @mejiaj @amyleadem

Currently, I'm leaning towards holding off on implementing this change since it has wider implications than just character count. I think it's a great candidate for #5884 which aims to normalize error states across all form components. I added a checklist item to that issue 👍

@mahoneycm mahoneycm mentioned this pull request Aug 23, 2024
@mahoneycm
Copy link
Contributor Author

mahoneycm commented Aug 23, 2024

Update 8/23

@mejiaj

This comment was marked as resolved.

mejiaj

This comment was marked as outdated.

Copy link

@jaclinec jaclinec left a comment

Choose a reason for hiding this comment

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

This looks great! Nice work!

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.

@mahoneycm Behavior and styles are looking good @mahoneycm!

I had one open question about the failure of the component when either .usa-label or .usa-form-group is missing. It feels a bit fragile to me. I'm wondering if we should at least mark it as a (potentially) breaking change so that users check that their implementations of the character count component have all the needed elements to work.

Curious what others think!

@amyleadem

This comment was marked as outdated.

@mahoneycm

This comment was marked as outdated.

mejiaj

This comment was marked as outdated.

@mahoneycm
Copy link
Contributor Author

How do we feel about the necessity for labels or form group elements?

Seems valid. Feels a little odd to check for valid form HTML, but it matches other patterns where we throw errors if the required component markup is missing.

My opinion
I'd prefer to rely on guidance, but I can also see a case for throwing an error.

This makes sense to me! I went ahead and removed the related error messages and unit tests! We can rely on guidance and example code for proper markup 👍

@mejiaj mejiaj self-requested a review September 23, 2024 21:08
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.

Confirming that this work displays the character count error state when markup is correct and user goes over limit.

Unit tests and builds pass. No console errors when interacting with the component.

character-count-5908-2024-09-24.at.11.00.41.webm

Comment on lines +31 to +37
// Disabling a11y tests from `npm run test:a11y` because we're demo'ing the failure intentionally.
export const TestNoLabel = TestNoLabelTemplate.bind({});
TestNoLabel.parameters = {
axe: {
mode: "off",
},
};
Copy link
Contributor

@mejiaj mejiaj Sep 24, 2024

Choose a reason for hiding this comment

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

Tip

This test intentionally fails a11y CI test to view the error state when markup is incorrect/missing.

Followed this guidance to disable a11y tests for those templates. 1

Footnotes

  1. https://github.com/chanzuckerberg/axe-storybook-testing?tab=readme-ov-file#disabledrules

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.

Looks good to me! Confirmed that the updated styles have appropriate enhancements that make the error state more visible and consistent with other USWDS styles

Screenshots:
image

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.

LGTM. Passing it over to @heymatthenry for his take.

Copy link
Contributor

@heymatthenry heymatthenry left a comment

Choose a reason for hiding this comment

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

LGTM! And a great PR, to boot—clear and thoughtful.

@heymatthenry heymatthenry merged commit ddc28d9 into develop Oct 4, 2024
5 checks passed
@heymatthenry heymatthenry deleted the cm-char-count-error-state branch October 4, 2024 18:07
@thisisdano thisisdano mentioned this pull request Oct 4, 2024
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 - Character count: Implement a visual cue when the limit is exceeded
7 participants