Use <label> HTML element when possible
#7609
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
UUID are cryptographically secure unique identifiers and take time to get generated. It's better to use the useId hook which is made for that purpose, always returns the same value for the same hook instance, and works well with server-side rendering.
…isabling an eslint rule
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request enhances accessibility and performance in input components by implementing the <label> HTML element and utilizing React's useId() hook.
- Replaced
<span>with<label>inTextInput.tsxfor improved accessibility - Implemented
useId()hook inCheckbox.tsx,Radio.tsx, andTextInputV2.tsxfor stable ID generation - Updated
Toggle.tsxto use functional state updates inuseEffectfor more reliable state management - Improved label-input linking across components using
htmlForattribute andid - Enhanced overall accessibility and adherence to best practices in React development
5 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -65,12 +65,15 @@ const TextInput: React.FC<TextInputProps> = ({ | |||
| placeholder, | |||
| icon, | |||
| }) => { | |||
| const inputId = useId(); | |||
There was a problem hiding this comment.
nice I didn't know about this one!
There was a problem hiding this comment.
This should be our default way to generated Ids in the frontend (when we don't need uuids). What do you think @lucasbordeau?
There was a problem hiding this comment.
The useId hook remains a React hook and must follow the hooks' rules. It can't be called in response to an event. The useId hook is excellent for generating identifiers scoped to a React component, like HTML IDs.
There was a problem hiding this comment.
I won't say that it should be the default way for all ids but for HTML ids it's most indicated.
charlesBochet
approved these changes
Oct 13, 2024
| @@ -165,7 +164,7 @@ export const Checkbox = ({ | |||
| setIsInternalChecked(event.target.checked ?? false); | |||
| }; | |||
|
|
|||
| const checkboxId = 'checkbox' + v4(); | |||
| const checkboxId = React.useId(); | |||
There was a problem hiding this comment.
The whole component imports hooks from the React global export, so I did the same.
Log |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
<label>HTML elements when possible to represent labelsuseId()React hook to get an identifier to link the label with its input; it's more suitable than generating a UUID at every renderFixes #7281