Skip to content

Commit 44bcdd7

Browse files
Lukas742Lukas742
authored
fix(Toolbar (compat)): exclude spacer from overflow popover (#6962)
Fixes #6933
1 parent 157c5be commit 44bcdd7

File tree

3 files changed

+47
-54
lines changed

3 files changed

+47
-54
lines changed

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

+34-41
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ interface OverflowPopoverProps {
2727
children: ReactNode[];
2828
portalContainer: Element;
2929
overflowContentRef: Ref<HTMLDivElement>;
30-
numberOfAlwaysVisibleItems?: number;
3130
overflowPopoverRef?: Ref<PopoverDomRef>;
3231
overflowButton?: ReactElement<ToggleButtonPropTypes> | ReactElement<ButtonPropTypes>;
3332
setIsMounted: Dispatch<SetStateAction<boolean>>;
@@ -43,7 +42,6 @@ export const OverflowPopover: FC<OverflowPopoverProps> = (props: OverflowPopover
4342
children,
4443
portalContainer,
4544
overflowContentRef,
46-
numberOfAlwaysVisibleItems,
4745
overflowButton,
4846
overflowPopoverRef,
4947
setIsMounted,
@@ -126,50 +124,45 @@ export const OverflowPopover: FC<OverflowPopoverProps> = (props: OverflowPopover
126124
})();
127125

128126
const OverflowPopoverContextProvider = getOverflowPopoverContext().Provider;
127+
const visibleChildren = lastVisibleIndex === -1 ? children : children.slice(lastVisibleIndex + 1);
129128

130-
let startIndex = null;
131-
const filteredChildrenArray = children
129+
const filteredChildren = (visibleChildren as ReactElement[])
130+
// @ts-expect-error: if type is not defined, it's not a spacer
131+
.filter((child) => child.type?.displayName !== 'ToolbarSpacer' && isValidElement(child))
132132
.map((item, index, arr) => {
133-
if (index > lastVisibleIndex && index > numberOfAlwaysVisibleItems - 1 && isValidElement(item)) {
134-
if (startIndex === null) {
135-
startIndex = index;
136-
}
137-
const labelProp = item?.props?.['data-accessible-name'] ? 'accessibleName' : 'aria-label';
138-
let labelVal = i18nBundle.getText(X_OF_Y, index + 1 - startIndex, arr.length - startIndex);
139-
if (item?.props?.[labelProp]) {
140-
labelVal += ' ' + item.props[labelProp];
141-
}
133+
const labelProp = item?.props?.['data-accessible-name'] ? 'accessibleName' : 'aria-label';
134+
let labelVal = i18nBundle.getText(X_OF_Y, index + 1, arr.length);
135+
if (item?.props?.[labelProp]) {
136+
labelVal += ' ' + item.props[labelProp];
137+
}
142138

143-
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
144-
// @ts-ignore: React 19
145-
if (item?.props?.id) {
146-
return cloneElement<HTMLAttributes<HTMLElement>>(item, {
147-
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
148-
// @ts-ignore: React 19
149-
id: `${item.props.id}-overflow`,
150-
[labelProp]: labelVal
151-
});
152-
}
153-
// @ts-expect-error: if type is not defined, it's not a spacer
154-
if (item.type?.displayName === 'ToolbarSeparator') {
155-
return cloneElement(item as ReactElement, {
156-
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
157-
// @ts-ignore: React 19
158-
style: {
159-
height: '0.0625rem',
160-
margin: '0.375rem 0.1875rem',
161-
width: '100%'
162-
},
163-
'aria-label': labelVal
164-
});
165-
}
139+
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
140+
// @ts-ignore: React 19
141+
if (item?.props?.id) {
166142
return cloneElement<HTMLAttributes<HTMLElement>>(item, {
143+
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
144+
// @ts-ignore: React 19
145+
id: `${item.props.id}-overflow`,
167146
[labelProp]: labelVal
168147
});
169148
}
170-
return null;
171-
})
172-
.filter(Boolean);
149+
// @ts-expect-error: if type is not defined, it's not a separator
150+
if (item.type?.displayName === 'ToolbarSeparator') {
151+
return cloneElement(item, {
152+
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
153+
// @ts-ignore: React 19
154+
style: {
155+
height: '0.0625rem',
156+
margin: '0.375rem 0.1875rem',
157+
width: '100%'
158+
},
159+
'aria-label': labelVal
160+
});
161+
}
162+
return cloneElement<HTMLAttributes<HTMLElement>>(item, {
163+
[labelProp]: labelVal
164+
});
165+
});
173166

174167
return (
175168
<OverflowPopoverContextProvider value={{ inPopover: true }}>
@@ -200,15 +193,15 @@ export const OverflowPopover: FC<OverflowPopoverProps> = (props: OverflowPopover
200193
onOpen={handleAfterOpen}
201194
hideArrow
202195
accessibleRole={accessibleRole}
203-
accessibleName={i18nBundle.getText(WITH_X_ITEMS, filteredChildrenArray.length)}
196+
accessibleName={i18nBundle.getText(WITH_X_ITEMS, filteredChildren.length)}
204197
>
205198
<div
206199
className={classes.popoverContent}
207200
ref={overflowContentRef}
208201
role={a11yConfig?.overflowPopover?.contentRole}
209202
data-component-name="ToolbarOverflowPopoverContent"
210203
>
211-
{filteredChildrenArray}
204+
{filteredChildren}
212205
</div>
213206
</Popover>,
214207
portalContainer ?? document.body

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

+12-11
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ const OverflowTestComponent = (props: PropTypes) => {
7979
<Text data-testid="toolbar-item" style={{ width: '200px' }}>
8080
Item1
8181
</Text>
82+
{/*not rendered in overflow popover*/}
8283
<ToolbarSpacer data-testid="spacer1" />
8384
<Text data-testid="toolbar-item2" style={{ width: '200px' }}>
8485
Item2
@@ -143,7 +144,7 @@ describe('Toolbar', () => {
143144
cy.mount(<OverflowTestComponent onOverflowChange={onOverflowChange} />);
144145
cy.get('@overflowChangeSpy').should('have.been.calledOnce');
145146
cy.findByTestId('toolbarElements').should('have.text', 2);
146-
cy.findByTestId('overflowElements').should('have.text', 4);
147+
cy.findByTestId('overflowElements').should('have.text', 3);
147148
cy.findByText('Item1').should('be.visible');
148149
cy.get('[data-testid="toolbar-item2"]').should('not.be.visible');
149150
cy.get('[data-testid="toolbar-item3"]').should('not.be.visible');
@@ -156,22 +157,22 @@ describe('Toolbar', () => {
156157

157158
cy.viewport(500, 500);
158159

159-
// fuzzy - remount component instead
160+
// flaky - remount component instead
160161
// cy.get(`[ui5-toggle-button]`).click();
161162
cy.mount(<OverflowTestComponent onOverflowChange={onOverflowChange} />);
162163
cy.get('[ui5-popover]').should('not.have.attr', 'open');
163164

164165
cy.get('@overflowChangeSpy').should('have.callCount', 2);
165166
cy.findByTestId('toolbarElements').should('have.text', 3);
166-
cy.findByTestId('overflowElements').should('have.text', 3);
167+
cy.findByTestId('overflowElements').should('have.text', 2);
167168

168169
cy.findByTestId('input').shadow().find('input').type('100');
169170
cy.findByTestId('input').trigger('change');
170171
cy.findByTestId('input').shadow().find('input').clear({ force: true });
171172

172173
cy.get('@overflowChangeSpy').should('have.callCount', 3);
173174
cy.findByTestId('toolbarElements').should('have.text', 0);
174-
cy.findByTestId('overflowElements').should('have.text', 6);
175+
cy.findByTestId('overflowElements').should('have.text', 4);
175176

176177
cy.get('[data-testid="toolbar-item"]').should('not.be.visible');
177178
cy.get('[data-testid="toolbar-item2"]').should('not.be.visible');
@@ -193,13 +194,13 @@ describe('Toolbar', () => {
193194

194195
cy.get('@overflowChangeSpy').should('have.callCount', 5);
195196
cy.findByTestId('toolbarElements').should('have.text', 3);
196-
cy.findByTestId('overflowElements').should('have.text', 3);
197+
cy.findByTestId('overflowElements').should('have.text', 2);
197198

198199
cy.findByText('Add').click();
199200

200201
cy.get('@overflowChangeSpy').should('have.callCount', 6);
201202
cy.findByTestId('toolbarElements').should('have.text', 3);
202-
cy.findByTestId('overflowElements').should('have.text', 4);
203+
cy.findByTestId('overflowElements').should('have.text', 3);
203204

204205
cy.findByText('Add').click();
205206
cy.findByText('Add').click();
@@ -209,13 +210,13 @@ describe('Toolbar', () => {
209210

210211
cy.get('@overflowChangeSpy').should('have.callCount', 11);
211212
cy.findByTestId('toolbarElements').should('have.text', 3);
212-
cy.findByTestId('overflowElements').should('have.text', 9);
213+
cy.findByTestId('overflowElements').should('have.text', 8);
213214

214215
cy.findByText('Remove').click();
215216

216217
cy.get('@overflowChangeSpy').should('have.callCount', 12);
217218
cy.findByTestId('toolbarElements').should('have.text', 3);
218-
cy.findByTestId('overflowElements').should('have.text', 8);
219+
cy.findByTestId('overflowElements').should('have.text', 7);
219220

220221
cy.findByText('Remove').click();
221222
cy.findByText('Remove').click();
@@ -225,14 +226,14 @@ describe('Toolbar', () => {
225226

226227
cy.get('@overflowChangeSpy').should('have.callCount', 17);
227228
cy.findByTestId('toolbarElements').should('have.text', 3);
228-
cy.findByTestId('overflowElements').should('have.text', 3);
229+
cy.findByTestId('overflowElements').should('have.text', 2);
229230

230231
cy.get(`[ui5-toggle-button]`).click();
231232

232-
// ToolbarSpacers should not be visible in the popover
233+
// ToolbarSpacers should not be rendered in the popover
233234
cy.get('[data-component-name="ToolbarOverflowPopover"]')
234235
.findByTestId('spacer2')
235-
.should('not.be.visible', { timeout: 100 });
236+
.should('not.exist', { timeout: 100 });
236237
cy.findByTestId('spacer1').should('exist');
237238

238239
// ToolbarSeparator should be displayed with horizontal line

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -400,11 +400,10 @@ const Toolbar = forwardRef<HTMLDivElement, ToolbarPropTypes>((props, ref) => {
400400
>
401401
<OverflowPopover
402402
overflowPopoverRef={overflowPopoverRef}
403-
lastVisibleIndex={lastVisibleIndex}
403+
lastVisibleIndex={Math.max(lastVisibleIndex, numberOfAlwaysVisibleItems - 1)}
404404
classes={classNames}
405405
portalContainer={portalContainer}
406406
overflowContentRef={overflowContentRef}
407-
numberOfAlwaysVisibleItems={numberOfAlwaysVisibleItems}
408407
overflowButton={overflowButton}
409408
setIsMounted={setIsPopoverMounted}
410409
a11yConfig={a11yConfig}

0 commit comments

Comments
 (0)