Skip to content

Conversation

@arn-craft
Copy link
Collaborator

No description provided.

@arn-craft arn-craft requested a review from mkrause October 10, 2025 13:07

/** The (inline) size of the list box. Optional. Default: `medium`. */
size?: undefined | 'shrink' | 'small' | 'medium' | 'large',
size?: undefined | 'shrink' | 'small' | 'medium' | 'large' | 'xl',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename xlx-large

requestFocus: () => void;
isSelected: boolean;
requestSelection: () => void;
selectedChildKey: string | null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you convert the ; to , here?

<div
className={cx(
cl['bk-list-box__group-layout'],
{ [cl['is-searching']]: isSearching },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should follow BEM: bk-list-box__group-layout--searching

}, [groups, selectedItemKey]);

// --- Layout selection ---
let content: React.ReactNode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move this section to a function? Like renderContent. Then we don't need the let (and the whole thing becomes nicely encapsulated)

React.isValidElement(child) &&
(child.props as OptionProps)?.itemKey === selectedItemKey
);
if (foundChild) return group;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add explicit braces if () { ... }. Also elsewhere

const results: OptionElement[] = [];

groups.forEach((group) => {
React.Children.forEach(group.props.children, (option) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather avoid JSX walking, this is deprecated in React.

@include bk.scroll-shadow($dir: start);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Trim the extra spaces here

&.bk-list-box__item--group, &.bk-list-box__item--option {
display: flex;
justify-content: space-between;
inline-size: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need the inline-size: 100%? Can this be converted to flex stretch?

/** Custom icon component. */
Icon?: undefined | ListBoxIcon,

info?: React.ReactNode,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add undefined | also

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants