Skip to content

feat(design-system): add DsSplitButton component [AR-54024]#347

Open
iromanchuk-dn wants to merge 9 commits intodrivenets:mainfrom
iromanchuk-dn:AR-54024-design-system-split-button
Open

feat(design-system): add DsSplitButton component [AR-54024]#347
iromanchuk-dn wants to merge 9 commits intodrivenets:mainfrom
iromanchuk-dn:AR-54024-design-system-split-button

Conversation

@vpolessky-dn
Copy link
Copy Markdown
Collaborator

missing browser test fro the button

export type SplitButtonSize = (typeof splitButtonSizes)[number];

export interface DsSplitButtonSlotProps {
button: Partial<DsButtonV3Props>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

type SplitButtonButtonProps = Pick<DsButtonV3Props, 'icon' | 'loading' | 'onClick' | 'aria-label' | 'className' | 'disabled'>;

$divider-z-index: $highlighted-z-index + 1;
$divider-width: 1px;
$border-width: 1px;
$button-transition-duration: 0.15s;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

transition standard is .2s

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I made it aligned with DsButtonV3, it's important to have the same duration, because $button-transition-duration affects button right border.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So maybe add a comment about this?
Or even better, let's try to find a way to share it and have a single source of truth
I'll just note that I'm afraid that importing it as a SCSS variable will import & bundle the whole button SCSS file, so need to be careful with that

'aria-label': 'Refresh',
onClick,
},
select: { ...defaultSelect, onValueChange: vi.fn() } as DsSelectProps,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you don't need any of the as DsSelectProps

value: '30',
onValueChange: vi.fn(),
multiple: false,
} as const satisfies Partial<DsSelectProps>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this could be simplified to:

Suggested change
} as const satisfies Partial<DsSelectProps>;
} satisfies DsSelectProps;

'aria-label': 'Refresh',
onClick,
},
select: { ...defaultSelect, onValueChange: vi.fn() } as DsSelectProps,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you're overriding an existing prop in all tests for no reason. This can be simplified in all cases:

Suggested change
select: { ...defaultSelect, onValueChange: vi.fn() } as DsSelectProps,
select: defaultSelect,

$divider-z-index: $highlighted-z-index + 1;
$divider-width: 1px;
$border-width: 1px;
$button-transition-duration: 0.15s;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So maybe add a comment about this?
Or even better, let's try to find a way to share it and have a single source of truth
I'll just note that I'm afraid that importing it as a SCSS variable will import & bundle the whole button SCSS file, so need to be careful with that

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like there are some padding-top issues with the small size (maybe bug in the select component itself?):
image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this component closed to a specific use case / composition?

That's not the what happens in MUI or Chakra

(also, maybe we could reconsider the naming here, according to the above examples)

Comment on lines +16 to +18
const { className: buttonClassName, disabled: buttonDisabled, ...buttonRest } = slotProps.button;

const { className: selectClassName, disabled: selectDisabled, ...selectRest } = slotProps.select;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick, non-blocker

maybe this is nicer? 😅

Suggested change
const { className: buttonClassName, disabled: buttonDisabled, ...buttonRest } = slotProps.button;
const { className: selectClassName, disabled: selectDisabled, ...selectRest } = slotProps.select;
const { className: buttonClassName, disabled: buttonDisabled, ...buttonProps } = slotProps.button;
const { className: selectClassName, disabled: selectDisabled, ...selectProps } = slotProps.select;

Comment on lines +10 to +11
type DistributiveOmit<T, K extends PropertyKey> = T extends unknown ? Omit<T, K> : never;
type SelectSlotProps = DistributiveOmit<DsSelectProps, 'size'>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

maybe at some point we'll have a type-utils.ts file
I guess we should keep this in mind once we'll add more utils like this

Comment on lines +4 to +6
export const getSelectSize = (size: SplitButtonSize): SelectSize => {
return size === 'medium' ? 'default' : 'small';
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO having a whole file for this is overkill
I would put this in the component itself as a variable 🤷

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants