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

[BUG] ButtonColor, OuiButtonIconColor typedef conflict #1196

Open
pjfitzgibbons opened this issue Jan 5, 2024 · 1 comment
Open

[BUG] ButtonColor, OuiButtonIconColor typedef conflict #1196

pjfitzgibbons opened this issue Jan 5, 2024 · 1 comment
Labels
bug Something isn't working untriaged

Comments

@pjfitzgibbons
Copy link
Contributor

Describe the bug
ButtonColor, OuiButtonIconColor have nearly overlapping definitions :

ButtonColor == Omit<OuiButtonIconColor, 'subdued'>
OuiButtonIconColor == Omit<ButtonColor, 'success'>

This mismatch though causes typedef conflict and failure of tsc --noEmit when a single color var is used to style sibling OuiButton and OuiButtonIcon.
See (#1193, split_button_control.tsx:113)

Expected behavior

A single unified color type that can be used between Button and ButtonIcon

Screenshots

Typescript language server in VSCode produces :

Type 'ButtonColor' is not assignable to type '"accent" | "danger" | "ghost" | "primary" | "subdued" | "success" | "text" | "warning" | undefined'.
  Type '"secondary"' is not assignable to type '"accent" | "danger" | "ghost" | "primary" | "subdued" | "success" | "text" | "warning" | undefined'.ts(2322)
button_icon.tsx(81, 3): The expected type comes from property 'color' which is declared here on type 'IntrinsicAttributes & PropsWithChildren<(DisambiguateSet<OuiButtonIconPropsForAnchor, OuiButtonIconPropsForButton> & ... 4 more ... & { ...; }) | (DisambiguateSet<...> & ... 4 more ... & { ...; })>'
(property) color: "accent" | "danger" | "ghost" | "primary" | "success" | "text" | "warning"

Host/Environment (please complete the following information):

  • OUI Version: [e.g. 1.0]
  • OSD Version (if applicable): [e.g. 2.7.0]
  • OS: [e.g. iOS]
  • Browser and version [e.g. Chrome 90]

Additional context

Add any other context about the problem here.

@pjfitzgibbons pjfitzgibbons added bug Something isn't working untriaged labels Jan 5, 2024
@joshuali925
Copy link
Member

joshuali925 commented Jan 5, 2024

it's because of

color?: ButtonColor;

You have
{...propsForDropdownButton}

the type is
propsForDropdownButton?: OuiButtonProps;

which can contain color?: ButtonColor
export interface OuiButtonProps extends OuiButtonContentProps, CommonProps {
children?: ReactNode;
/**
* Make button a solid color for prominence
*/
fill?: boolean;
/**
* Any of our named colors.
* **`secondary` color is DEPRECATED, use `success` instead**
*/
color?: ButtonColor;

it can't be used for color in OuiButtonIcon because that component expects OuiButtonIconColor.
export interface OuiButtonIconProps extends CommonProps {
iconType: IconType;
/**
* Any of the named color palette options.
* **`subdued` set to be DEPRECATED, use `text` instead**
*/
color?: OuiButtonIconColor;

You can move color={color} after the property spread, or (better) define type for propsForDropdownButton as something like propsForDropdownButton?: Omit<OuiButtonProps, 'color'>;

Edit: add more details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working untriaged
Projects
None yet
Development

No branches or pull requests

2 participants