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.

Sign up for GitHub

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

Jump to bottom

v3: Menu is using ListItem #1295

Merged
merged 29 commits into from
Jun 1, 2023
Merged

v3: Menu is using ListItem #1295

Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
0c78c71
Updating MenuItem to use ListItem
gretanausedaite May 18, 2023
f956dc4
unit test fix
gretanausedaite May 22, 2023
e19701e
CSS tests update
gretanausedaite May 24, 2023
bf9a178
Finally fixed img version of MenuItem icon
gretanausedaite May 24, 2023
11c8da3
Merge branch 'dev' into greta/new-menu-item
gretanausedaite May 24, 2023
43ef956
Merge remote-tracking branch 'origin/dev' into greta/new-menu-item
gretanausedaite May 25, 2023
93007f8
Prop deprecate not remove
gretanausedaite May 25, 2023
7b1ec0d
fix skeleton a bit
gretanausedaite May 25, 2023
b29704b
remove select cloneElement
gretanausedaite May 25, 2023
aead592
focused prop instead of data attr
gretanausedaite May 25, 2023
808956f
Merge remote-tracking branch 'origin/dev' into greta/new-menu-item
gretanausedaite May 29, 2023
d252b52
undo select changes
gretanausedaite May 30, 2023
b317fee
fix tests?
gretanausedaite May 30, 2023
b6cf131
Merge branch 'dev' into greta/new-menu-item
gretanausedaite May 31, 2023
eaad312
fix icon
gretanausedaite May 31, 2023
6f5db0f
Updating css for skeleton
gretanausedaite May 31, 2023
5f5be5e
unit tests fix
gretanausedaite May 31, 2023
ec2ee58
css skeleton fix
gretanausedaite May 31, 2023
11cfc01
changeset
gretanausedaite May 31, 2023
46524e9
changeset
gretanausedaite May 31, 2023
e57c9e2
Undo the undo
gretanausedaite May 31, 2023
d1d596f
Update .changeset/quick-dodos-hang.md
gretanausedaite May 31, 2023
14fcace
Rename storybook tests
gretanausedaite May 31, 2023
740700b
not needed
gretanausedaite May 31, 2023
e427b4d
Merge branch 'dev' into greta/new-menu-item
gretanausedaite Jun 1, 2023
e8c2feb
docs update
gretanausedaite Jun 1, 2023
cf4520c
docs update
gretanausedaite Jun 1, 2023
c116a56
Updating for comments
gretanausedaite Jun 1, 2023
b9e0021
Merge branch 'dev' into greta/new-menu-item
gretanausedaite Jun 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 41 additions & 42 deletions packages/itwinui-react/src/core/Select/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import * as React from 'react';
import cx from 'classnames';
import { Menu, MenuItem } from '../Menu/index.js';
import type { MenuItemProps } from '../Menu/MenuItem.js';
import {
SvgCaretDownSmall,
Popover,
Expand Down Expand Up @@ -314,50 +313,50 @@ export const Select = <T,>(props: SelectProps<T>): JSX.Element => {

const startIcon = startIconProp ?? icon;

const menuItem: JSX.Element = itemRenderer ? (
return itemRenderer ? (
itemRenderer(option, { close: () => setIsOpen(false), isSelected })
) : (
<MenuItem startIcon={startIcon}>{label}</MenuItem>
<MenuItem
startIcon={startIcon}
key={`${label}-${index}`}
isSelected={isSelected}
onClick={() => {
if (option.disabled) {
return;
}
if (isSingleOnChange(onChange, multiple)) {
onChange?.(option.value);
setIsOpen(false);
} else {
onChange?.(option.value, isSelected ? 'removed' : 'added');
}

// update live region
if (isMultipleEnabled(value, multiple)) {
const prevSelectedValue = value || [];
const newSelectedValue = isSelected
? prevSelectedValue.filter((i) => option.value !== i)
: [...prevSelectedValue, option.value];
setLiveRegionSelection(
options
.filter((i) => newSelectedValue.includes(i.value))
.map((item) => item.label)
.filter(Boolean)
.join(', '),
);
}
}}
ref={(el: HTMLLIElement) => {
if (isSelected && !multiple) {
el?.scrollIntoView({ block: 'nearest' });
}
}}
role='option'
{...restOption}
>
{label}
</MenuItem>
);

return React.cloneElement<MenuItemProps>(menuItem, {
Copy link
Contributor

Choose a reason for hiding this comment

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

the original reason for using cloneElement here was for adding extra stuff to menuItem returned by custom itemRenderer.

so if we're removing it, there needs to be a way to expose equivalent functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking if we need to remove this cloneElement statement?
Reverted changes for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's ok to keep for now because we are not passing className inside it. we can try to figure out a better solution in the future

key: `${label}-${index}`,
isSelected,
onClick: () => {
if (option.disabled) {
return;
}
if (isSingleOnChange(onChange, multiple)) {
onChange?.(option.value);
setIsOpen(false);
} else {
onChange?.(option.value, isSelected ? 'removed' : 'added');
}

// update live region
if (isMultipleEnabled(value, multiple)) {
const prevSelectedValue = value || [];
const newSelectedValue = isSelected
? prevSelectedValue.filter((i) => option.value !== i)
: [...prevSelectedValue, option.value];
setLiveRegionSelection(
options
.filter((i) => newSelectedValue.includes(i.value))
.map((item) => item.label)
.filter(Boolean)
.join(', '),
);
}
},
ref: (el: HTMLElement) => {
if (isSelected && !multiple) {
el?.scrollIntoView({ block: 'nearest' });
}
},
role: 'option',
...restOption,
...menuItem.props,
});
});
}, [itemRenderer, multiple, onChange, options, value]);

Expand Down