-
Notifications
You must be signed in to change notification settings - Fork 647
Adds character counts to TextArea and TextInput components #7293
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: dbf0f21 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
| {...rest} | ||
| aria-describedby={ | ||
| characterLimit | ||
| ? [characterCountStaticMessageId, rest['aria-describedby']].filter(Boolean).join(' ') || undefined |
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.
We needed to add the characterCountStaticMessageId as an aria-describedby ID to ensure that the user is getting the information about the character limit.
| <VisuallyHidden id={characterCountLiveRegionId} aria-live="polite" role="status"> | ||
| {screenReaderMessage} | ||
| </VisuallyHidden> | ||
| <VisuallyHidden id={characterCountStaticMessageId}> |
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 is hidden as we want to include it for screen reader users, but including the actual character counter span that updates when a user types was causing double announcements.
| <UnstyledTextInput | ||
| // @ts-expect-error it needs a non nullable ref | ||
| ref={inputRef} | ||
| <> |
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.
I just wrapped the <TextInputWrapper> in an empty <> since we're adding another sibling component. I'll call out specific changes within this part of the diff!
siddharthkp
left a comment
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.
Trust you with the implementation overall. Left a couple small nits.
Next steps:
- Add e2e tests + add
update snapshotslabel - Run integration tests
| const [isInputFocused, setIsInputFocused] = useState<boolean>(false) | ||
| const inputRef = useProvidedRefOrCreate(ref as React.RefObject<HTMLInputElement | null>) | ||
| const [characterCount, setCharacterCount] = useState<string>('') | ||
| const [isOverLimit, setIsOverLimit] = useState<boolean>(false) |
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.
nit: Can this be inferred instead of stored in state? Might save us a re-render?
const isOverLimit = characterCount > characterLimit
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.
I tried storing it both as a ref and a constant and updating it in a useEffect, but since it needs to update on every keystroke it wasn't quite working for me. Do you have another suggestion or do you think it's ok as-is? I see you added it's a "nit" so wanted to check!
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.
yep, called it a nit because I assumed you must have had a reason.
but since it needs to update on every keystroke it wasn't quite working for me.
that's odd, I assumed setCharacterCount would cause a re-render which can be used to calculate isOverLimit.
anywho, not ideal but also not blocking
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/9302 |
|
I see that one of the checks is still pending here but completed in the integration branch 🤔 Going to add |
Related to https://github.com/github/primer/issues/5937.
Related PVC PR
Below is a video showcasing that TextArea and TextField components have an optional
characterLimitthat can be passed in as a prop. When a user types, the field is updated with how many characters are left / how many are over. When a user exceeds the limit, the character count text changes to red and an error icon prepends the text.There is also an
aria-liveregion that updates after a slight delay (500ms) when a user finishes typing. This accurately tells screen reader users how many characters they have left / are over, as well as when the input is invalid if they have typed too many characters.We are also including
sr-onlytext that is associated with the input so that when a user focuses on the input, they hear "You can enter up to X character(s)". This was added because associating the "X character(s) remaining" message was causing duplicate announcements in NVDA and this is the recommended approach.Components._.Textarea._.Features.-.With.Character.Limit.Storybook.-.19.December.2025.mp4
Changelog
New
characterLimitprop toTextAreacharacterLimitprop toTextInputcharacter_counter.tsfunctionalityChanged
Removed
Rollout strategy
Testing & Reviewing
Merge checklist