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

refactor: [M3-6708] – Implement new React 18 hooks #10261

Merged
merged 5 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions docs/development-guide/13-coding-standards.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,28 @@ If you are using VSCode it is highly recommended to use the ESlint extension. Th

## React

- When conditionally rendering JSX, use ternaries instead of `&&`.
- Example: `condition ? <Component /> : null` instead of `condition && <Component />`
- This is to avoid hard-to-catch bugs ([read more](https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx)).
Comment on lines -18 to -20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We moved away from this convention about a year ago, but this was lingering in our Coding Standards.

[Several new hooks were introduced with the release of React 18](https://react.dev/blog/2022/03/29/react-v18#new-hooks).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If/when other newly-introduced hooks get used in the code base, perhaps guidelines/explanations around their usage could be included in this section also.


It should be noted that the `useId()` hook is particularly useful for generating unique IDs for accessibility attributes. For this use case, `useId()` is preferred over hardcoding the ID because components may be rendered more than once on a page, but IDs must be unique.

As an example from `DisplayLinodes.tsx`, early in the file we invoke the hook: `const displayViewDescriptionId = React.useId()`

And make use of the unique ID by passing it as the value for a component's `aria-describedby` attribute in the `return` value:

```
<StyledToggleButton
aria-describedby={displayViewDescriptionId}
aria-label="Toggle display"
disableRipple
isActive={true}
onClick={toggleLinodeView}
size="large"
>
<GridView />
</StyledToggleButton>
```

Per the [docs](https://react.dev/reference/react/useId#usage), the hook should not be used for generating keys in a list.

## Event Handler Naming Convention

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Tech Stories
---

Implement new useId() hook in several components ([#10261](https://github.com/linode/manager/pull/10261))
6 changes: 4 additions & 2 deletions packages/manager/src/components/GroupByTagToggle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ interface GroupByTagToggleProps {
export const GroupByTagToggle = React.memo((props: GroupByTagToggleProps) => {
const { isGroupedByTag, isLargeAccount, toggleGroupByTag } = props;

const groupByDescriptionId = React.useId();

return (
<>
<div className="visually-hidden" id="groupByDescription">
<div className="visually-hidden" id={groupByDescriptionId}>
{isGroupedByTag
? 'group by tag is currently enabled'
: 'group by tag is currently disabled'}
Expand All @@ -25,7 +27,7 @@ export const GroupByTagToggle = React.memo((props: GroupByTagToggleProps) => {
title={`${isGroupedByTag ? 'Ungroup' : 'Group'} by tag`}
>
<StyledToggleButton
aria-describedby={'groupByDescription'}
aria-describedby={groupByDescriptionId}
aria-label={`Toggle group by tag`}
disableRipple
// See https://github.com/linode/manager/pull/6653 for more details
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ export const LinodeConfigDialog = (props: Props) => {

const { enqueueSnackbar } = useSnackbar();

const virtModeCaptionId = React.useId();

const {
data: kernels,
error: kernelsError,
Expand Down Expand Up @@ -721,7 +723,7 @@ export const LinodeConfigDialog = (props: Props) => {
<Typography variant="h3">Virtual Machine</Typography>
<FormControl>
<FormLabel
aria-describedby="virtModeCaption"
aria-describedby={virtModeCaptionId}
disabled={isReadOnly}
htmlFor="virt_mode"
>
Expand All @@ -745,7 +747,7 @@ export const LinodeConfigDialog = (props: Props) => {
label="Full virtualization"
value="fullvirt"
/>
<FormHelperText id="virtModeCaption">
<FormHelperText id={virtModeCaptionId}>
Controls if devices inside your virtual machine are
paravirtualized or fully virtualized. Paravirt is what you
want, unless you&rsquo;re doing weird things.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ export const DisplayGroupedLinodes = (props: CombinedProps) => {
...rest
} = props;

const displayViewDescriptionId = React.useId();
const groupByDescriptionId = React.useId();

const dataLength = data.length;

const orderedGroupedLinodes = compose(sortGroups, groupByTags)(data);
Expand All @@ -96,12 +99,12 @@ export const DisplayGroupedLinodes = (props: CombinedProps) => {
<>
<Grid className={'px0'} xs={12}>
<StyledControlHeader isGroupedByTag={linodesAreGrouped}>
<div className="visually-hidden" id="displayViewDescription">
<div className="visually-hidden" id={displayViewDescriptionId}>
Currently in {linodeViewPreference} view
</div>
<Tooltip placement="top" title="List view">
<StyledToggleButton
aria-describedby={'displayViewDescription'}
aria-describedby={displayViewDescriptionId}
aria-label="Toggle display"
disableRipple
isActive={linodesAreGrouped}
Expand All @@ -112,14 +115,14 @@ export const DisplayGroupedLinodes = (props: CombinedProps) => {
</StyledToggleButton>
</Tooltip>

<div className="visually-hidden" id="groupByDescription">
<div className="visually-hidden" id={groupByDescriptionId}>
{linodesAreGrouped
? 'group by tag is currently enabled'
: 'group by tag is currently disabled'}
</div>
<Tooltip placement="top-end" title="Ungroup by tag">
<StyledToggleButton
aria-describedby={'groupByDescription'}
aria-describedby={groupByDescriptionId}
aria-label={`Toggle group by tag`}
disableRipple
isActive={linodesAreGrouped}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ export const DisplayLinodes = React.memo((props: CombinedProps) => {
...rest
} = props;

const displayViewDescriptionId = React.useId();
const groupByDescriptionId = React.useId();

const { infinitePageSize, setInfinitePageSize } = useInfinitePageSize();
const numberOfLinodesWithMaintenance = React.useMemo(() => {
return data.reduce((acc, thisLinode) => {
Expand Down Expand Up @@ -143,13 +146,13 @@ export const DisplayLinodes = React.memo((props: CombinedProps) => {
<StyledControlHeader isGroupedByTag={linodesAreGrouped}>
<div
className="visually-hidden"
id="displayViewDescription"
id={displayViewDescriptionId}
>
Currently in {linodeViewPreference} view
</div>
<Tooltip placement="top" title="List view">
<StyledToggleButton
aria-describedby={'displayViewDescription'}
aria-describedby={displayViewDescriptionId}
aria-label="Toggle display"
disableRipple
isActive={true}
Expand All @@ -160,14 +163,14 @@ export const DisplayLinodes = React.memo((props: CombinedProps) => {
</StyledToggleButton>
</Tooltip>

<div className="visually-hidden" id="groupByDescription">
<div className="visually-hidden" id={groupByDescriptionId}>
{linodesAreGrouped
? 'group by tag is currently enabled'
: 'group by tag is currently disabled'}
</div>
<Tooltip placement="top-end" title="Group by tag">
<StyledToggleButton
aria-describedby={'groupByDescription'}
aria-describedby={groupByDescriptionId}
aria-label={`Toggle group by tag`}
disableRipple
isActive={linodesAreGrouped}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ export const SortableTableHead = <T extends unknown>(
toggleLinodeView,
} = props;

const displayViewDescriptionId = React.useId();

const isActive = (label: string) =>
label.toLowerCase() === orderBy.toLowerCase();

Expand Down Expand Up @@ -167,12 +169,12 @@ export const SortableTableHead = <T extends unknown>(
justifyContent: 'flex-end',
}}
>
<div className="visually-hidden" id="displayViewDescription">
<div className="visually-hidden" id={displayViewDescriptionId}>
Currently in {linodeViewPreference} view
</div>
<Tooltip placement="top" title="Summary view">
<StyledToggleButton
aria-describedby={'displayViewDescription'}
aria-describedby={displayViewDescriptionId}
aria-label="Toggle display"
disableRipple
isActive={linodeViewPreference === 'grid'}
Expand Down
Loading