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

Fix ComboBox options' tag flicking #2120

Draft
wants to merge 74 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
74 commits
Select commit Hold shift + click to select a range
78c44a4
Trying different approach in useOverflow
r100-stack Jun 27, 2024
8c7dd69
Fixed bug causing lower than correct visibleCount.
r100-stack Jun 27, 2024
0c4187a
Bug fixes. Handle resize.
r100-stack Jun 28, 2024
b938917
Slight code cleanup
r100-stack Jun 28, 2024
e41fc01
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jun 28, 2024
4d42668
Tried to remove items prop. Better disabled handling.
r100-stack Jul 2, 2024
d09c68d
Fix infinite loop when no overflow
r100-stack Jul 2, 2024
c2f6b14
Update min guess when doubling.
r100-stack Jul 3, 2024
8418ab0
Remove optimization for resize
r100-stack Jul 3, 2024
faab06e
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 3, 2024
8467fac
Fix incorrect reset
r100-stack Jul 3, 2024
6d37971
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 3, 2024
24f5957
Left a TODO from the debugging with edge cases
r100-stack Jul 3, 2024
1ac77c5
Avoid necessary reguessing on resize
r100-stack Jul 3, 2024
2b70f31
Trying the component approach
r100-stack Jul 5, 2024
728e62e
Working resize and tag location center
r100-stack Jul 5, 2024
67dda3b
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 5, 2024
7aafdc3
Add resize to leftover overflow examples
r100-stack Jul 5, 2024
ffcb17e
Bring back image tests
r100-stack Jul 5, 2024
c8dcc20
Merge branch 'r/overflow-add-resize-leftover' into r/combobox-selecte…
r100-stack Jul 5, 2024
c5789cc
Working, but…, useOverflow doesn't listen to resize.
r100-stack Jul 8, 2024
7835190
Fixed the bug
r100-stack Jul 8, 2024
f52b103
Added `minVisibleCount` to `OverflowContainer` for `Breadcrumbs`
r100-stack Jul 8, 2024
6f1acf1
Generalized calculation
r100-stack Jul 8, 2024
9be9adb
Move OverflowContainer to a separate file
r100-stack Jul 9, 2024
2ac3263
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 10, 2024
017d4c3
Fix some errors
r100-stack Jul 10, 2024
88b20f9
Fix imports
r100-stack Jul 10, 2024
8641377
Using elements instead of refs
r100-stack Jul 10, 2024
0294267
useEffect doesn't run continuously
r100-stack Jul 10, 2024
6a77e1c
Working Breadcrumbs
r100-stack Jul 10, 2024
f738339
Cleanup
r100-stack Jul 10, 2024
cce04e9
Fix jumping UI in the beginning
r100-stack Jul 10, 2024
063dc3f
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 11, 2024
1581039
Easier solution than setting container from outside
r100-stack Jul 11, 2024
5afb58e
Working MiddleTextTruncation
r100-stack Jul 11, 2024
345d913
Working for horizontal ButtonGroup
r100-stack Jul 11, 2024
e692a0f
Added disabled & orientation to OverflowContainer
r100-stack Jul 11, 2024
e48dce7
Fixed incorrect ButtonGroup firstOverflowIndex.
r100-stack Jul 12, 2024
b417c5e
Working for TablePaginator
r100-stack Jul 12, 2024
cbf924c
Cleanup
r100-stack Jul 12, 2024
8e689b9
`overflowTag` is required if children is an array
r100-stack Jul 12, 2024
56612df
JSDocs
r100-stack Jul 12, 2024
d1e1a56
Remove unnecessary try-catch
r100-stack Jul 12, 2024
fc2c7fd
Removed unnecessary props from OverflowContainer
r100-stack Jul 12, 2024
0dee6a3
Better disabled support
r100-stack Jul 12, 2024
ccdd5a8
Revert overflowButton playground changes
r100-stack Jul 12, 2024
f815339
WIP e2e tests
r100-stack Jul 15, 2024
815d876
Revert storybook
r100-stack Jul 16, 2024
2be10a1
Remove testing hooks
r100-stack Jul 16, 2024
64f461e
Move useOverflow to OverflowContainer
r100-stack Jul 16, 2024
a96ee46
Remaining e2e tests
r100-stack Jul 16, 2024
bca632a
Reverted playgrounds
r100-stack Jul 16, 2024
6d59b82
Resolve TODOs
r100-stack Jul 16, 2024
152e52a
Resolve TODOs in OverflowContainer
r100-stack Jul 16, 2024
2ad671a
Fix flaky test
r100-stack Jul 16, 2024
7e529b3
Changeset
r100-stack Jul 16, 2024
506dc70
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 16, 2024
0fe9e31
Clean/Fix e2e tests
r100-stack Jul 16, 2024
5c25794
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 16, 2024
eb97cbf
Set STARTING_MAX_ITEMS_COUNT = 32
r100-stack Jul 16, 2024
5926d74
Cleanup
r100-stack Jul 16, 2024
c3e75ff
Remove unnecessary changes
r100-stack Jul 16, 2024
1bca804
Moved failing unit tests to e2e
r100-stack Jul 17, 2024
fde3af2
Leftover failing unit tests to e2e
r100-stack Jul 17, 2024
f612b2f
More failing unit tests to e2e
r100-stack Jul 17, 2024
adbb684
Changeset and nits
r100-stack Jul 17, 2024
4bc78aa
Merge remote-tracking branch 'origin/main' into r/combobox-selected-o…
r100-stack Jul 17, 2024
7284b66
More explanation
r100-stack Jul 17, 2024
8c0c8de
Remove unnecessary changes.
r100-stack Jul 17, 2024
cef8d02
Remove accidental duplicate DOM components
r100-stack Jul 17, 2024
9555771
Fix failing e2e test
r100-stack Jul 17, 2024
bafa64b
leftover
r100-stack Jul 17, 2024
10bb0a5
Fix e2e tests failing only in non-UI mode
r100-stack Jul 17, 2024
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
5 changes: 5 additions & 0 deletions .changeset/light-poems-trade.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@itwin/itwinui-css': minor
---

`iui-breadcrumbs-list` now stretches to the width of `iui-breadcrumbs`.
5 changes: 5 additions & 0 deletions .changeset/mean-mangos-relax.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@itwin/itwinui-react': patch
---

Fixed a bug in `ComboBox` with `multiple` enabled where the number of tags used to keep changing in an infinite loop in certain specific container widths. As a result of this fix, the overflow behavior has been improved in other components too (e.g. `ButtonGroup`, `Breadcrumbs`, `MiddleTextTruncation`, etc.)
1 change: 1 addition & 0 deletions packages/itwinui-css/src/breadcrumbs/breadcrumbs.scss
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
list-style-type: none;
user-select: none;
block-size: 100%;
inline-size: 100%;
Copy link
Member Author

@r100-stack r100-stack Jul 16, 2024

Choose a reason for hiding this comment

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

Needed this to allow the .iui-breadcrumbs-list's visibleCount to update whenever .iui-breadcrumbs resizes.

}

.iui-breadcrumbs-item {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,10 @@
* See LICENSE.md in the project root for license terms and full copyright notice.
*--------------------------------------------------------------------------------------------*/
import * as React from 'react';
import { fireEvent, render, screen } from '@testing-library/react';
import { render, screen } from '@testing-library/react';

import { Breadcrumbs } from './Breadcrumbs.js';
import { SvgChevronRight, SvgMore } from '../../utils/index.js';
import * as UseOverflow from '../../utils/hooks/useOverflow.js';
import { IconButton } from '../Buttons/IconButton.js';
import { SvgChevronRight } from '../../utils/index.js';
import { Button } from '../Buttons/Button.js';
import { userEvent } from '@testing-library/user-event';

Expand Down Expand Up @@ -56,13 +54,6 @@ const assertBaseElement = (
});
};

const useOverflowMock = vi
.spyOn(UseOverflow, 'useOverflow')
.mockImplementation((items) => [vi.fn(), items.length]);
beforeEach(() => {
useOverflowMock.mockImplementation((items) => [vi.fn(), items.length]);
});

it('should render all elements in default state', () => {
const { container } = renderComponent();
assertBaseElement(container);
Expand Down Expand Up @@ -181,57 +172,6 @@ it('should accept currentIndex prop', () => {
assertBaseElement(container, { currentIndex: 1 });
});

it('should overflow when there is not enough space', () => {
useOverflowMock.mockReturnValue([vi.fn(), 2]);
const { container } = renderComponent();

expect(container.querySelector('.iui-breadcrumbs')).toBeTruthy();
expect(container.querySelector('.iui-breadcrumbs-list')).toBeTruthy();

const breadcrumbs = container.querySelectorAll('.iui-breadcrumbs-item');
expect(breadcrumbs.length).toEqual(3);
expect(breadcrumbs[0].textContent).toEqual('Item 0');
expect(breadcrumbs[1].textContent).toEqual('…');
expect(breadcrumbs[1].firstElementChild?.textContent).toContain('…');
expect(breadcrumbs[2].textContent).toEqual('Item 2');
});

it('should handle overflow when overflowButton is specified', () => {
const onClick = vi.fn();
useOverflowMock.mockReturnValue([vi.fn(), 2]);
const { container } = renderComponent({
overflowButton: (visibleCount) => (
<IconButton onClick={onClick(visibleCount)}>
<SvgMore />
</IconButton>
),
});

expect(container.querySelector('.iui-breadcrumbs')).toBeTruthy();
expect(container.querySelector('.iui-breadcrumbs-list')).toBeTruthy();

const breadcrumbs = container.querySelectorAll('.iui-breadcrumbs-item');
expect(breadcrumbs.length).toEqual(3);
fireEvent.click(breadcrumbs[1]);
expect(onClick).toHaveBeenCalledTimes(1);
expect(onClick).toHaveBeenCalledWith(2);
});

it('should show the last item when only one can be visible', () => {
useOverflowMock.mockReturnValue([vi.fn(), 1]);

const { container } = renderComponent();

expect(container.querySelector('.iui-breadcrumbs')).toBeTruthy();
expect(container.querySelector('.iui-breadcrumbs-list')).toBeTruthy();

const breadcrumbs = container.querySelectorAll('.iui-breadcrumbs-item');
expect(breadcrumbs.length).toEqual(2);
expect(breadcrumbs[0].textContent).toEqual('…');
expect(breadcrumbs[0].firstElementChild?.textContent).toContain('…');
expect(breadcrumbs[1].textContent).toEqual('Item 2');
});

it('should support legacy api', async () => {
const onClick = vi.fn();

Expand Down
98 changes: 51 additions & 47 deletions packages/itwinui-react/src/core/Breadcrumbs/Breadcrumbs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@
import * as React from 'react';
import cx from 'classnames';
import {
useMergedRefs,
useOverflow,
SvgChevronRight,
Box,
createWarningLogger,
} from '../../utils/index.js';
import type { PolymorphicForwardRefComponent } from '../../utils/index.js';
import { Button } from '../Buttons/Button.js';
import { Anchor } from '../Typography/Anchor.js';
import { OverflowContainer } from '../../utils/components/OverflowContainer.js';

const logWarning = createWarningLogger();

Expand Down Expand Up @@ -124,62 +123,67 @@ const BreadcrumbsComponent = React.forwardRef((props, ref) => {
...rest
} = props;

const [overflowRef, visibleCount] = useOverflow(items);
const refs = useMergedRefs(overflowRef, ref);

return (
<Box
as='nav'
className={cx('iui-breadcrumbs', className)}
ref={refs}
ref={ref}
aria-label='Breadcrumb'
{...rest}
>
<Box as='ol' className='iui-breadcrumbs-list'>
{visibleCount > 1 && (
<>
<ListItem item={items[0]} isActive={currentIndex === 0} />
<Separator separator={separator} />
</>
)}
{items.length - visibleCount > 0 && (
<OverflowContainer
as='ol'
className='iui-breadcrumbs-list'
itemsLength={items.length}
>
{(visibleCount: number) => (
<>
<Box as='li' className='iui-breadcrumbs-item'>
{overflowButton ? (
overflowButton(visibleCount)
) : (
<Box as='span' className='iui-breadcrumbs-content'>
{visibleCount > 1 && (
<>
<ListItem item={items[0]} isActive={currentIndex === 0} />
<Separator separator={separator} />
</>
)}
{items.length - visibleCount > 0 && (
<>
<Box as='li' className='iui-breadcrumbs-item'>
{overflowButton ? (
overflowButton(visibleCount)
) : (
<Box as='span' className='iui-breadcrumbs-content'>
</Box>
)}
</Box>
)}
</Box>
<Separator separator={separator} />
<Separator separator={separator} />
</>
)}
{items
.slice(
visibleCount > 1
? items.length - visibleCount + 1
: items.length - 1,
)
.map((_, _index) => {
const index =
visibleCount > 1
? 1 + (items.length - visibleCount) + _index
: items.length - 1;
return (
<React.Fragment key={index}>
<ListItem
item={items[index]}
isActive={currentIndex === index}
/>
{index < items.length - 1 && (
<Separator separator={separator} />
)}
</React.Fragment>
);
})}
</>
)}
{items
.slice(
visibleCount > 1
? items.length - visibleCount + 1
: items.length - 1,
)
.map((_, _index) => {
const index =
visibleCount > 1
? 1 + (items.length - visibleCount) + _index
: items.length - 1;
return (
<React.Fragment key={index}>
<ListItem
item={items[index]}
isActive={currentIndex === index}
/>
{index < items.length - 1 && (
<Separator separator={separator} />
)}
</React.Fragment>
);
})}
</Box>
</OverflowContainer>
</Box>
);
}) as PolymorphicForwardRefComponent<'nav', BreadcrumbsProps>;
Expand Down
141 changes: 1 addition & 140 deletions packages/itwinui-react/src/core/ButtonGroup/ButtonGroup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@
* Copyright (c) Bentley Systems, Incorporated. All rights reserved.
* See LICENSE.md in the project root for license terms and full copyright notice.
*--------------------------------------------------------------------------------------------*/
import { SvgMore, SvgClose as SvgPlaceholder } from '../../utils/index.js';
import { fireEvent, render } from '@testing-library/react';
import * as UseOverflow from '../../utils/hooks/useOverflow.js';
import { render } from '@testing-library/react';

import { ButtonGroup } from './ButtonGroup.js';
import { Button } from '../Buttons/Button.js';
import { IconButton } from '../Buttons/IconButton.js';

it('should render with two buttons', () => {
const { container } = render(
Expand All @@ -34,101 +31,6 @@ it('should render with four buttons', () => {
expect(container.querySelectorAll('button').length).toBe(4);
});

it.each(['start', 'end'] as const)(
'should handle overflow when overflowButton is specified (%s)',
(overflowPlacement) => {
const scrollWidthSpy = vi
.spyOn(HTMLElement.prototype, 'scrollWidth', 'get')
.mockReturnValueOnce(250)
.mockReturnValue(200);
const offsetWidthSpy = vi
.spyOn(HTMLElement.prototype, 'offsetWidth', 'get')
.mockReturnValue(200);

const mockOnClick = vi.fn();

const OverflowGroup = () => {
const buttons = [...Array(3)].map((_, index) => (
<IconButton key={index}>
<SvgPlaceholder />
</IconButton>
));
return (
<ButtonGroup
overflowButton={(overflowStart) => (
<IconButton onClick={() => mockOnClick(overflowStart)}>
<SvgMore />
</IconButton>
)}
overflowPlacement={overflowPlacement}
>
{buttons}
</ButtonGroup>
);
};
const { container } = render(<OverflowGroup />);

expect(container.querySelector('.iui-button-group')).toBeTruthy();

const buttons = container.querySelectorAll('.iui-button');
expect(buttons).toHaveLength(2);
fireEvent.click(overflowPlacement === 'end' ? buttons[1] : buttons[0]);
expect(mockOnClick).toHaveBeenCalledWith(1);

scrollWidthSpy.mockRestore();
offsetWidthSpy.mockRestore();
},
);

it.each(['start', 'end'] as const)(
'should handle overflow when available space is smaller than one element (%s)',
(overflowPlacement) => {
const scrollWidthSpy = vi
.spyOn(HTMLElement.prototype, 'scrollWidth', 'get')
.mockReturnValue(200);
const offsetWidthSpy = vi
.spyOn(HTMLElement.prototype, 'offsetWidth', 'get')
.mockReturnValue(50);

const OverflowGroup = () => {
const buttons = [...Array(3)].map((_, index) => (
<IconButton key={index}>
<SvgPlaceholder />
</IconButton>
));
return (
<ButtonGroup
overflowButton={() => (
<IconButton>
<SvgMore />
</IconButton>
)}
overflowPlacement={overflowPlacement}
>
{buttons}
</ButtonGroup>
);
};
const { container } = render(<OverflowGroup />);
const {
container: { firstChild: moreIconButton },
} = render(
<IconButton>
<SvgMore />
</IconButton>,
);

expect(container.querySelector('.iui-button-group')).toBeTruthy();

const buttons = container.querySelectorAll('.iui-button');
expect(buttons).toHaveLength(1);
expect(buttons[0]).toEqual(moreIconButton);

scrollWidthSpy.mockRestore();
offsetWidthSpy.mockRestore();
},
);

it('should work in vertical orientation', () => {
const { container } = render(
<ButtonGroup orientation='vertical'>
Expand All @@ -142,44 +44,3 @@ it('should work in vertical orientation', () => {
expect(group).toBeTruthy();
expect(group.children).toHaveLength(2);
});

it.each`
visibleCount | overflowStart | length | overflowPlacement
${9} | ${1} | ${10} | ${'start'}
${8} | ${2} | ${10} | ${'start'}
${4} | ${6} | ${10} | ${'start'}
${3} | ${7} | ${10} | ${'start'}
${1} | ${9} | ${10} | ${'start'}
${9} | ${8} | ${10} | ${'end'}
${8} | ${7} | ${10} | ${'end'}
${4} | ${3} | ${10} | ${'end'}
${3} | ${2} | ${10} | ${'end'}
${1} | ${0} | ${10} | ${'end'}
`(
'should calculate correct values when overflowPlacement=$overflowPlacement and visibleCount=$visibleCount',
({ visibleCount, overflowStart, length, overflowPlacement }) => {
const useOverflowMock = vi
.spyOn(UseOverflow, 'useOverflow')
.mockReturnValue([vi.fn(), visibleCount]);

const buttons = [...Array(length)].map((_, index) => (
<button key={index}>{index}</button>
));

const { container } = render(
<ButtonGroup
overflowButton={(startIndex) => <span>{startIndex}</span>}
overflowPlacement={overflowPlacement}
>
{buttons}
</ButtonGroup>,
);

expect(container.querySelectorAll('button')).toHaveLength(visibleCount - 1);
expect(container.querySelector('span')).toHaveTextContent(
`${overflowStart}`,
);

useOverflowMock.mockRestore();
},
);
Loading
Loading