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 · 2 comments
Open

[BUG] ButtonColor, OuiButtonIconColor typedef conflict #1196

pjfitzgibbons opened this issue Jan 5, 2024 · 2 comments
Labels
bug Something isn't working

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

@dblock
Copy link
Member

dblock commented Jun 17, 2024

@joshuali925 @pjfitzgibbons Want to PR a fix?

Catch All Triage - 1 2 3 4 5

@dblock dblock removed the untriaged label Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants