Skip to content

Commit c546981

Browse files
reidbarberLFDanLu
reidbarber
and
LFDanLu
authored
Improve TagGroup rendering (#4496)
* reduce flickering in react 18 via flushSync * use maxHeight instead of flushSync * remove unused peerDep * cleanup * reduce getComputedStyle calls by hard-coding * add provider to ssr test * address comments --------- Co-authored-by: Daniel Lu <[email protected]>
1 parent e770d45 commit c546981

File tree

3 files changed

+102
-70
lines changed

3 files changed

+102
-70
lines changed

‎packages/@react-spectrum/tag/src/TagGroup.tsx

Lines changed: 50 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,25 @@ import {FocusRing, FocusScope} from '@react-aria/focus';
1919
// @ts-ignore
2020
import intlMessages from '../intl/*.json';
2121
import {ListCollection, useListState} from '@react-stately/list';
22-
import {Provider, useProviderProps} from '@react-spectrum/provider';
22+
import {Provider, useProvider, useProviderProps} from '@react-spectrum/provider';
2323
import React, {ReactElement, useCallback, useEffect, useMemo, useRef, useState} from 'react';
2424
import styles from '@adobe/spectrum-css-temp/components/tags/vars.css';
2525
import {Tag} from './Tag';
2626
import {useFormProps} from '@react-spectrum/form';
2727
import {useId, useLayoutEffect, useResizeObserver, useValueEffect} from '@react-aria/utils';
2828
import {useLocale, useLocalizedStringFormatter} from '@react-aria/i18n';
2929

30+
const TAG_STYLES = {
31+
medium: {
32+
height: 24,
33+
margin: 4
34+
},
35+
large: {
36+
height: 30,
37+
margin: 5
38+
}
39+
};
40+
3041
export interface SpectrumTagGroupProps<T> extends Omit<AriaTagGroupProps<T>, 'keyboardDelegate'>, StyleProps, SpectrumLabelableProps, Omit<SpectrumHelpTextProps, 'showErrorIcon'> {
3142
/** The label to display on the action button. */
3243
actionLabel?: string,
@@ -53,10 +64,11 @@ function TagGroup<T extends object>(props: SpectrumTagGroupProps<T>, ref: DOMRef
5364
let containerRef = useRef(null);
5465
let tagsRef = useRef(null);
5566
let {direction} = useLocale();
67+
let {scale} = useProvider();
5668
let stringFormatter = useLocalizedStringFormatter(intlMessages);
5769
let [isCollapsed, setIsCollapsed] = useState(maxRows != null);
5870
let state = useListState(props);
59-
let [tagState, setTagState] = useValueEffect({visibleTagCount: state.collection.size, showCollapseButton: false, maxHeight: undefined});
71+
let [tagState, setTagState] = useValueEffect({visibleTagCount: state.collection.size, showCollapseButton: false});
6072
let keyboardDelegate = useMemo(() => (
6173
isCollapsed
6274
? new TagKeyboardDelegate(new ListCollection([...state.collection].slice(0, tagState.visibleTagCount)), direction)
@@ -73,24 +85,19 @@ function TagGroup<T extends object>(props: SpectrumTagGroupProps<T>, ref: DOMRef
7385
// Refs can be null at runtime.
7486
let currContainerRef: HTMLDivElement | null = containerRef.current;
7587
let currTagsRef: HTMLDivElement | null = tagsRef.current;
76-
if (!currContainerRef || !currTagsRef) {
77-
return;
78-
}
79-
80-
let tags = [...currTagsRef.children];
81-
let buttons = [...currContainerRef.parentElement.querySelectorAll('button')];
82-
if (tags.length === 0 || buttons.length === 0) {
88+
if (!currContainerRef || !currTagsRef || state.collection.size === 0) {
8389
return {
8490
visibleTagCount: 0,
85-
showCollapseButton: false,
86-
maxHeight: undefined
91+
showCollapseButton: false
8792
};
8893
}
94+
95+
// Count rows and show tags until we hit the maxRows.
96+
let tags = [...currTagsRef.children];
8997
let currY = -Infinity;
9098
let rowCount = 0;
9199
let index = 0;
92100
let tagWidths: number[] = [];
93-
// Count rows and show tags until we hit the maxRows.
94101
for (let tag of tags) {
95102
let {width, y} = tag.getBoundingClientRect();
96103

@@ -107,36 +114,37 @@ function TagGroup<T extends object>(props: SpectrumTagGroupProps<T>, ref: DOMRef
107114
}
108115

109116
// Remove tags until there is space for the collapse button and action button (if present) on the last row.
110-
let buttonsWidth = buttons.reduce((acc, curr) => acc += curr.getBoundingClientRect().width, 0);
111-
buttonsWidth += parseInt(window.getComputedStyle(buttons[buttons.length - 1]).marginRight, 10) * 2;
112-
let end = direction === 'ltr' ? 'right' : 'left';
113-
let containerEnd = currContainerRef.parentElement.getBoundingClientRect()[end];
114-
let lastTagEnd = tags[index - 1]?.getBoundingClientRect()[end];
115-
lastTagEnd += parseInt(window.getComputedStyle(tags[index - 1]).marginRight, 10);
116-
let availableWidth = containerEnd - lastTagEnd;
117+
let buttons = [...currContainerRef.parentElement.querySelectorAll('button')];
118+
if (buttons.length > 0) {
119+
let buttonsWidth = buttons.reduce((acc, curr) => acc += curr.getBoundingClientRect().width, 0);
120+
buttonsWidth += parseInt(window.getComputedStyle(buttons[buttons.length - 1]).marginRight, 10) * 2;
121+
let end = direction === 'ltr' ? 'right' : 'left';
122+
let containerEnd = currContainerRef.parentElement.getBoundingClientRect()[end];
123+
let lastTagEnd = tags[index - 1]?.getBoundingClientRect()[end];
124+
lastTagEnd += TAG_STYLES[scale].margin;
125+
let availableWidth = containerEnd - lastTagEnd;
117126

118-
while (availableWidth < buttonsWidth && index < state.collection.size && index > 0) {
119-
availableWidth += tagWidths.pop();
120-
index--;
127+
while (availableWidth < buttonsWidth && index < state.collection.size && index > 0) {
128+
availableWidth += tagWidths.pop();
129+
index--;
130+
}
121131
}
122-
let tagStyle = window.getComputedStyle(tags[0]);
123-
let maxHeight = (parseInt(tagStyle.height, 10) + parseInt(tagStyle.marginTop, 10) * 2) * maxRows;
132+
124133
return {
125-
visibleTagCount: index,
126-
showCollapseButton: index < state.collection.size,
127-
maxHeight
134+
visibleTagCount: Math.max(index, 1),
135+
showCollapseButton: index < state.collection.size
128136
};
129137
};
130138

131139
setTagState(function *() {
132140
// Update to show all items.
133-
yield {visibleTagCount: state.collection.size, showCollapseButton: true, maxHeight: undefined};
141+
yield {visibleTagCount: state.collection.size, showCollapseButton: true};
134142

135143
// Measure, and update to show the items until maxRows is reached.
136144
yield computeVisibleTagCount();
137145
});
138146
}
139-
}, [maxRows, setTagState, direction, state.collection.size]);
147+
}, [maxRows, setTagState, direction, scale, state.collection.size]);
140148

141149
useResizeObserver({ref: containerRef, onResize: updateVisibleTagCount});
142150
// eslint-disable-next-line react-hooks/exhaustive-deps
@@ -148,10 +156,10 @@ function TagGroup<T extends object>(props: SpectrumTagGroupProps<T>, ref: DOMRef
148156
// eslint-disable-next-line react-hooks/exhaustive-deps
149157
}, []);
150158

151-
let visibleTags = [...state.collection];
152-
if (maxRows != null && isCollapsed) {
153-
visibleTags = visibleTags.slice(0, tagState.visibleTagCount);
154-
}
159+
let visibleTags = useMemo(() =>
160+
[...state.collection].slice(0, isCollapsed ? tagState.visibleTagCount : state.collection.size),
161+
[isCollapsed, state.collection, tagState.visibleTagCount]
162+
);
155163

156164
let handlePressCollapse = () => {
157165
// Prevents button from losing focus if focusedKey got collapsed.
@@ -162,6 +170,14 @@ function TagGroup<T extends object>(props: SpectrumTagGroupProps<T>, ref: DOMRef
162170
let showActions = tagState.showCollapseButton || (actionLabel && onAction);
163171
let isEmpty = state.collection.size === 0;
164172

173+
let containerStyle = useMemo(() => {
174+
if (maxRows == null || !isCollapsed) {
175+
return undefined;
176+
}
177+
let maxHeight = (TAG_STYLES[scale].height + (TAG_STYLES[scale].margin * 2)) * maxRows;
178+
return {maxHeight, overflow: 'hidden'};
179+
}, [isCollapsed, maxRows, scale]);
180+
165181
return (
166182
<FocusScope>
167183
<Field
@@ -182,8 +198,8 @@ function TagGroup<T extends object>(props: SpectrumTagGroupProps<T>, ref: DOMRef
182198
)
183199
}>
184200
<div
185-
style={maxRows != null && tagState.showCollapseButton && isCollapsed ? {maxHeight: tagState.maxHeight, overflow: 'hidden'} : undefined}
186201
ref={containerRef}
202+
style={containerStyle}
187203
className={
188204
classNames(
189205
styles,

‎packages/@react-spectrum/tag/test/TagGroup.ssr.test.js

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,16 @@ describe('TagGroup SSR', function () {
1616
it('should render without errors', async function () {
1717
await testSSR(__filename, `
1818
import {TagGroup, Item} from '../';
19-
<TagGroup aria-label="Static TagGroup items example">
20-
<Item>News</Item>
21-
<Item>Travel</Item>
22-
<Item>Gaming</Item>
23-
<Item>Shopping</Item>
24-
</TagGroup>
19+
import {Provider} from '@react-spectrum/provider';
20+
import {theme} from '@react-spectrum/theme-default';
21+
<Provider theme={theme}>
22+
<TagGroup aria-label="Static TagGroup items example">
23+
<Item>News</Item>
24+
<Item>Travel</Item>
25+
<Item>Gaming</Item>
26+
<Item>Shopping</Item>
27+
</TagGroup>
28+
</Provider>
2529
`);
2630
});
2731
});

‎packages/@react-spectrum/tag/test/TagGroup.test.js

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,13 @@ describe('TagGroup', function () {
6464

6565
it('provides context for Tag component', function () {
6666
let {getAllByRole} = render(
67-
<TagGroup aria-label="tag group" allowsRemoving onRemove={onRemoveSpy}>
68-
<Item aria-label="Tag 1">Tag 1</Item>
69-
<Item aria-label="Tag 2">Tag 2</Item>
70-
<Item aria-label="Tag 3">Tag 3</Item>
71-
</TagGroup>
67+
<Provider theme={theme}>
68+
<TagGroup aria-label="tag group" allowsRemoving onRemove={onRemoveSpy}>
69+
<Item aria-label="Tag 1">Tag 1</Item>
70+
<Item aria-label="Tag 2">Tag 2</Item>
71+
<Item aria-label="Tag 3">Tag 3</Item>
72+
</TagGroup>
73+
</Provider>
7274
);
7375

7476
let tags = getAllByRole('row');
@@ -81,10 +83,12 @@ describe('TagGroup', function () {
8183

8284
it('has correct accessibility roles', () => {
8385
let {getByRole, getAllByRole} = render(
84-
<TagGroup
85-
aria-label="tag group">
86-
<Item aria-label="Tag 1">Tag 1</Item>
87-
</TagGroup>
86+
<Provider theme={theme}>
87+
<TagGroup
88+
aria-label="tag group">
89+
<Item aria-label="Tag 1">Tag 1</Item>
90+
</TagGroup>
91+
</Provider>
8892
);
8993

9094
let tagGroup = getByRole('grid');
@@ -96,10 +100,12 @@ describe('TagGroup', function () {
96100

97101
it('has correct tab index', () => {
98102
let {getAllByRole} = render(
99-
<TagGroup
100-
aria-label="tag group">
101-
<Item aria-label="Tag 1">Tag 1</Item>
102-
</TagGroup>
103+
<Provider theme={theme}>
104+
<TagGroup
105+
aria-label="tag group">
106+
<Item aria-label="Tag 1">Tag 1</Item>
107+
</TagGroup>
108+
</Provider>
103109
);
104110

105111
let tags = getAllByRole('row');
@@ -132,9 +138,11 @@ describe('TagGroup', function () {
132138

133139
it('TagGroup allows aria-label', function () {
134140
let {getByRole} = render(
135-
<TagGroup aria-label="tag group">
136-
<Item key="1" aria-label="Tag 1">Tag 1</Item>
137-
</TagGroup>
141+
<Provider theme={theme}>
142+
<TagGroup aria-label="tag group">
143+
<Item key="1" aria-label="Tag 1">Tag 1</Item>
144+
</TagGroup>
145+
</Provider>
138146
);
139147

140148
let tagGroup = getByRole('grid');
@@ -143,9 +151,11 @@ describe('TagGroup', function () {
143151

144152
it('TagGroup allows aria-labelledby', function () {
145153
let {getByRole} = render(
146-
<TagGroup aria-labelledby="tag group">
147-
<Item key="1" aria-label="Tag 1">Tag 1</Item>
148-
</TagGroup>
154+
<Provider theme={theme}>
155+
<TagGroup aria-labelledby="tag group">
156+
<Item key="1" aria-label="Tag 1">Tag 1</Item>
157+
</TagGroup>
158+
</Provider>
149159
);
150160

151161
let tagGroup = getByRole('grid');
@@ -154,11 +164,13 @@ describe('TagGroup', function () {
154164

155165
it('TagGroup allows aria-label on Item', function () {
156166
let {getByRole} = render(
157-
<TagGroup aria-label="tag group">
158-
<Item key="1" aria-label="Tag 1">Tag 1</Item>
159-
<Item key="2" aria-label="Tag 2">Tag 2</Item>
160-
<Item key="3" aria-label="Tag 3">Tag 3</Item>
161-
</TagGroup>
167+
<Provider theme={theme}>
168+
<TagGroup aria-label="tag group">
169+
<Item key="1" aria-label="Tag 1">Tag 1</Item>
170+
<Item key="2" aria-label="Tag 2">Tag 2</Item>
171+
<Item key="3" aria-label="Tag 3">Tag 3</Item>
172+
</TagGroup>
173+
</Provider>
162174
);
163175

164176
let tagGroup = getByRole('grid');
@@ -481,7 +493,7 @@ describe('TagGroup', function () {
481493
.mockImplementationOnce(() => ({x: 200, y: 400, width: 95, height: 32, top: 400, right: 290, bottom: 435, left: 200}))
482494
.mockImplementationOnce(() => ({x: 200, y: 300, width: 200, height: 128, top: 300, right: 400, bottom: 435, left: 200}))
483495
.mockImplementationOnce(() => ({x: 265, y: 335, width: 75, height: 32, top: 335, right: 345, bottom: 370, left: 265}));
484-
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px', marginTop: '4px', height: '24px'}));
496+
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px'}));
485497

486498
let {getAllByRole, getByRole} = render(
487499
<Provider theme={theme}>
@@ -524,7 +536,7 @@ describe('TagGroup', function () {
524536
.mockImplementationOnce(() => ({x: 200, y: 335, width: 65, height: 32, top: 335, right: 265, bottom: 370, left: 200}))
525537
.mockImplementationOnce(() => ({x: 265, y: 335, width: 75, height: 32, top: 335, right: 345, bottom: 370, left: 265}))
526538
.mockImplementationOnce(() => ({x: 200, y: 370, width: 120, height: 32, top: 370, right: 320, bottom: 400, left: 200}));
527-
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px', marginTop: '4px', height: '24px'}));
539+
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px'}));
528540
let {getAllByRole, queryAllByRole} = render(
529541
<Provider theme={theme}>
530542
<TagGroup maxRows={2} aria-label="tag group">
@@ -556,7 +568,7 @@ describe('TagGroup', function () {
556568
};
557569
}
558570
];
559-
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px', marginTop: '4px', height: '24px'}));
571+
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px'}));
560572
mockImplementation(target, mockCalls, true);
561573
let {getAllByRole, getByRole} = render(
562574
<Provider theme={theme}>
@@ -615,7 +627,7 @@ describe('TagGroup', function () {
615627
.mockImplementationOnce(() => ({x: 200, y: 300, width: 200, height: 128, top: 300, right: 400, bottom: 435, left: 200}))
616628
.mockImplementationOnce(() => ({x: 265, y: 335, width: 75, height: 32, top: 335, right: 345, bottom: 370, left: 265}))
617629
.mockImplementationOnce(() => ({x: 200, y: 300, width: 75, height: 32, top: 300, right: 275, bottom: 335, left: 200}));
618-
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px', marginTop: '4px', height: '24px'}));
630+
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px'}));
619631
let {getAllByRole} = render(
620632
<Provider theme={theme}>
621633
<TagGroup
@@ -672,7 +684,7 @@ describe('TagGroup', function () {
672684
});
673685

674686
it('action group is labelled correctly', function () {
675-
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px', marginTop: '4px', height: '24px'}));
687+
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px'}));
676688

677689
let {getByRole} = render(
678690
<Provider theme={theme}>
@@ -710,7 +722,7 @@ describe('TagGroup', function () {
710722
});
711723

712724
it('should allow you to tab into TagGroup if empty with link', async function () {
713-
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px', marginTop: '4px', height: '24px'}));
725+
let computedStyles = jest.spyOn(window, 'getComputedStyle').mockImplementation(() => ({marginRight: '4px'}));
714726

715727
let renderEmptyState = () => (
716728
<span>No tags. <Link><a href="//react-spectrum.com">Click here</a></Link> to add some.</span>

0 commit comments

Comments
 (0)