-
Notifications
You must be signed in to change notification settings - Fork 1.1k
USWDS - Character count: Enhance visual cue when limit is exceeded #5908
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
…sts; add no label and no form group templates
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.
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:
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}`); | ||
} | ||
|
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.
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.
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.
Removed errors in fe42118
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}`; |
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.
Note
Added classes and elements used to add error states. Organized by when they appear in markup/DOM
FORM_GROUP_ERROR_CLASS, | ||
LABEL_ERROR_CLASS, | ||
INPUT_ERROR_CLASS, |
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.
Note
Exported for use in new unit tests
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.
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.
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.
Note
New unit test to check for the existence of form group element
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.
todo: Remove unit test if we remove related errors
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.
Note
New unit test to check for the existence of the label element
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.
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); |
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.
Note
Adds / removes input error classes to match invalid status message class
@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 |
@amycole501 Here's what it looks like it with an offset of 2px Kapture.2024-05-03.at.15.59.41.webmThe 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 |
I like it! Would like to know what @alex-hull thinks but the separation makes it much easier to see the two borders. |
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
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.
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 👍 |
Update 8/23
|
This comment was marked as resolved.
This comment was marked as resolved.
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.
This looks great! Nice work!
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 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!
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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 👍 |
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.
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
// 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", | ||
}, | ||
}; |
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.
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
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.
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. Passing it over to @heymatthenry for his take.
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! And a great PR, to boot—clear and thoughtful.
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
Testing and review
Character count behaviors
Testing checklist
npm run test
runs without errorusa-input--error
classes are applied on character input over character limitusa-input--error
classes are removed on character input under character limitusa-form-group--error
classes are applied on character input over character limitusa-form-group--error
classes are removed on character input under character limitusa-label--error
classes are applied on character input over character limitusa-label--error
classes are removed on character input under character limitUnit tests match USWDS code standards// Unit tests removed