Skip to content

Commit 73d5065

Browse files
Lukas742Lukas742
authored
fix(Toolbar): return correct value for overflowElements in onOverflowChange (#4610)
This PR also fixes an issue where `onOverflowChange` was called indefinitely.
1 parent dcdaeaf commit 73d5065

File tree

3 files changed

+53
-8
lines changed

3 files changed

+53
-8
lines changed

‎packages/main/src/components/Toolbar/OverflowPopover.tsx

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import iconOverflow from '@ui5/webcomponents-icons/dist/overflow.js';
22
import { Device, useSyncRef } from '@ui5/webcomponents-react-base';
33
import { clsx } from 'clsx';
4-
import type { FC, ReactElement, ReactNode, Ref } from 'react';
4+
import type { Dispatch, FC, ReactElement, ReactNode, Ref, SetStateAction } from 'react';
55
import React, { cloneElement, useEffect, useRef, useState } from 'react';
66
import { createPortal } from 'react-dom';
77
import { ButtonDesign, PopoverPlacementType } from '../../enums/index.js';
@@ -27,6 +27,7 @@ interface OverflowPopoverProps {
2727
showMoreText: string;
2828
overflowPopoverRef?: Ref<PopoverDomRef>;
2929
overflowButton?: ReactElement<ToggleButtonPropTypes> | ReactElement<ButtonPropTypes>;
30+
setIsMounted: Dispatch<SetStateAction<boolean>>;
3031
}
3132

3233
const isPhone = Device.isPhone();
@@ -40,13 +41,21 @@ export const OverflowPopover: FC<OverflowPopoverProps> = (props: OverflowPopover
4041
overflowContentRef,
4142
numberOfAlwaysVisibleItems,
4243
showMoreText,
44+
overflowButton,
4345
overflowPopoverRef,
44-
overflowButton
46+
setIsMounted
4547
} = props;
4648
const [pressed, setPressed] = useState(false);
4749
const toggleBtnRef = useRef<ToggleButtonDomRef>(null);
4850
const [componentRef, popoverRef] = useSyncRef(overflowPopoverRef);
4951

52+
useEffect(() => {
53+
setIsMounted(true);
54+
return () => {
55+
setIsMounted(false);
56+
};
57+
}, []);
58+
5059
const handleToggleButtonClick = (e) => {
5160
e.stopPropagation();
5261
setPressed((prev) => {

‎packages/main/src/components/Toolbar/Toolbar.cy.tsx

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,16 @@ const OverflowTestComponent = (props: PropTypes) => {
3030
const { onOverflowChange } = props;
3131
const [width, setWidth] = useState(undefined);
3232
const [additionalChildren, setAdditionalChildren] = useState([]);
33+
const [eventProperties, setEventProperties] = useState({
34+
toolbarElements: [],
35+
overflowElements: undefined,
36+
target: undefined
37+
});
38+
39+
const handleOverflowChange = (e) => {
40+
onOverflowChange(e);
41+
setEventProperties(e);
42+
};
3343
return (
3444
<>
3545
<Input
@@ -62,7 +72,7 @@ const OverflowTestComponent = (props: PropTypes) => {
6272
</Button>
6373
<Toolbar
6474
data-testid="toolbar"
65-
onOverflowChange={onOverflowChange}
75+
onOverflowChange={handleOverflowChange}
6676
style={width ? { width: `${width}px`, maxWidth: 'none' } : undefined}
6777
>
6878
<Text data-testid="toolbar-item" style={{ width: '200px' }}>
@@ -79,6 +89,9 @@ const OverflowTestComponent = (props: PropTypes) => {
7989
{additionalChildren}
8090
<ToolbarSeparator data-testid="separator" />
8191
</Toolbar>
92+
<br />
93+
toolbarElements: <span data-testid="toolbarElements">{eventProperties.toolbarElements.length}</span>
94+
overflowElements: <span data-testid="overflowElements">{eventProperties.overflowElements?.length}</span>
8295
</>
8396
);
8497
};
@@ -128,6 +141,8 @@ describe('Toolbar', () => {
128141
cy.viewport(300, 500);
129142
cy.mount(<OverflowTestComponent onOverflowChange={onOverflowChange} />);
130143
cy.get('@overflowChangeSpy').should('have.been.calledOnce');
144+
cy.findByTestId('toolbarElements').should('have.text', 2);
145+
cy.findByTestId('overflowElements').should('have.text', 4);
131146
cy.findByText('Item1').should('be.visible');
132147
cy.get('[data-testid="toolbar-item2"]').should('not.be.visible');
133148
cy.get('[data-testid="toolbar-item3"]').should('not.be.visible');
@@ -146,12 +161,16 @@ describe('Toolbar', () => {
146161
cy.get('[ui5-popover]').should('not.have.attr', 'open');
147162

148163
cy.get('@overflowChangeSpy').should('have.callCount', 2);
164+
cy.findByTestId('toolbarElements').should('have.text', 3);
165+
cy.findByTestId('overflowElements').should('have.text', 3);
149166

150167
cy.findByTestId('input').shadow().find('input').type('100');
151168
cy.findByTestId('input').trigger('change');
152169
cy.findByTestId('input').shadow().find('input').clear({ force: true });
153170

154171
cy.get('@overflowChangeSpy').should('have.callCount', 3);
172+
cy.findByTestId('toolbarElements').should('have.text', 0);
173+
cy.findByTestId('overflowElements').should('have.text', 6);
155174

156175
cy.get('[data-testid="toolbar-item"]').should('not.be.visible');
157176
cy.get('[data-testid="toolbar-item2"]').should('not.be.visible');
@@ -161,6 +180,8 @@ describe('Toolbar', () => {
161180
cy.findByTestId('input').trigger('change');
162181

163182
cy.get('@overflowChangeSpy').should('have.callCount', 4);
183+
cy.findByTestId('toolbarElements').should('have.text', 6);
184+
cy.findByTestId('overflowElements').should('not.have.text');
164185

165186
cy.get('[data-testid="toolbar-item"]').should('be.visible');
166187
cy.get('[data-testid="toolbar-item2"]').should('be.visible');
@@ -170,10 +191,14 @@ describe('Toolbar', () => {
170191
cy.findByTestId('input').trigger('change');
171192

172193
cy.get('@overflowChangeSpy').should('have.callCount', 5);
194+
cy.findByTestId('toolbarElements').should('have.text', 3);
195+
cy.findByTestId('overflowElements').should('have.text', 3);
173196

174197
cy.findByText('Add').click();
175198

176199
cy.get('@overflowChangeSpy').should('have.callCount', 6);
200+
cy.findByTestId('toolbarElements').should('have.text', 3);
201+
cy.findByTestId('overflowElements').should('have.text', 4);
177202

178203
cy.findByText('Add').click();
179204
cy.findByText('Add').click();
@@ -182,10 +207,14 @@ describe('Toolbar', () => {
182207
cy.findByText('Add').click();
183208

184209
cy.get('@overflowChangeSpy').should('have.callCount', 11);
210+
cy.findByTestId('toolbarElements').should('have.text', 3);
211+
cy.findByTestId('overflowElements').should('have.text', 9);
185212

186213
cy.findByText('Remove').click();
187214

188215
cy.get('@overflowChangeSpy').should('have.callCount', 12);
216+
cy.findByTestId('toolbarElements').should('have.text', 3);
217+
cy.findByTestId('overflowElements').should('have.text', 8);
189218

190219
cy.findByText('Remove').click();
191220
cy.findByText('Remove').click();
@@ -194,6 +223,8 @@ describe('Toolbar', () => {
194223
cy.findByText('Remove').click();
195224

196225
cy.get('@overflowChangeSpy').should('have.callCount', 17);
226+
cy.findByTestId('toolbarElements').should('have.text', 3);
227+
cy.findByTestId('overflowElements').should('have.text', 3);
197228

198229
cy.get(`[ui5-toggle-button]`).click();
199230

‎packages/main/src/components/Toolbar/index.tsx

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ export interface ToolbarPropTypes extends Omit<CommonProps, 'onClick' | 'childre
9696
*/
9797
onOverflowChange?: (event: {
9898
toolbarElements: HTMLElement[];
99-
overflowElements: HTMLCollection;
99+
overflowElements: HTMLCollection | undefined;
100100
target: HTMLElement;
101101
}) => void;
102102
}
@@ -109,7 +109,7 @@ const OVERFLOW_BUTTON_WIDTH = 36 + 8 + 8; // width + padding end + spacing start
109109
* The content of the `Toolbar` moves into the overflow area from right to left when the available space is not enough in the visible area of the container.
110110
* It can be accessed by the user through the overflow button that opens it in a popover.
111111
*
112-
* __Note:__ The overflow popover is mounted only when opened, i.e., any child component of the popover will be remounted, when moved into it.
112+
* __Note:__ The overflow popover is mounted only when the overflow button is displayed, i.e., any child component of the popover will be remounted, when moved into it.
113113
*/
114114
const Toolbar = forwardRef<HTMLDivElement, ToolbarPropTypes>((props, ref) => {
115115
const {
@@ -134,6 +134,7 @@ const Toolbar = forwardRef<HTMLDivElement, ToolbarPropTypes>((props, ref) => {
134134
const [componentRef, outerContainer] = useSyncRef<HTMLDivElement>(ref);
135135
const controlMetaData = useRef([]);
136136
const [lastVisibleIndex, setLastVisibleIndex] = useState<number>(null);
137+
const [isPopoverMounted, setIsPopoverMounted] = useState(false);
137138
const contentRef = useRef(null);
138139
const overflowContentRef = useRef(null);
139140
const overflowBtnRef = useRef(null);
@@ -291,11 +292,14 @@ const Toolbar = forwardRef<HTMLDivElement, ToolbarPropTypes>((props, ref) => {
291292

292293
useEffect(() => {
293294
const haveChildrenChanged = prevChildren.current.length !== flatChildren.length;
294-
if ((lastVisibleIndex !== null || haveChildrenChanged) && typeof onOverflowChange === 'function') {
295+
if ((lastVisibleIndex !== null || haveChildrenChanged) && typeof debouncedOverflowChange.current === 'function') {
295296
prevChildren.current = flatChildren;
296297
const toolbarChildren = contentRef.current?.children;
297298
let toolbarElements = [];
298-
const overflowElements = overflowContentRef.current?.children;
299+
let overflowElements;
300+
if (isPopoverMounted) {
301+
overflowElements = overflowContentRef.current?.children;
302+
}
299303
if (toolbarChildren?.length > 0) {
300304
toolbarElements = Array.from(toolbarChildren).filter((item, index) => index <= lastVisibleIndex);
301305
}
@@ -308,7 +312,7 @@ const Toolbar = forwardRef<HTMLDivElement, ToolbarPropTypes>((props, ref) => {
308312
return () => {
309313
debouncedOverflowChange.current.cancel();
310314
};
311-
}, [lastVisibleIndex, flatChildren, debouncedOverflowChange]);
315+
}, [lastVisibleIndex, flatChildren.length, isPopoverMounted]);
312316

313317
const CustomTag = as as ElementType;
314318
const styleWithMinWidth = minWidth !== '0' ? { minWidth, ...style } : style;
@@ -352,6 +356,7 @@ const Toolbar = forwardRef<HTMLDivElement, ToolbarPropTypes>((props, ref) => {
352356
numberOfAlwaysVisibleItems={numberOfAlwaysVisibleItems}
353357
showMoreText={showMoreText}
354358
overflowButton={overflowButton}
359+
setIsMounted={setIsPopoverMounted}
355360
>
356361
{flatChildren}
357362
</OverflowPopover>

0 commit comments

Comments
 (0)