-
Notifications
You must be signed in to change notification settings - Fork 376
Description
Describe the issue. What is the expected and unexpected behavior?
Related to #3674
Looks like the button's icon prop only works if variant === ButtonVariant.link. I confirmed with design that any button should support the icon prop.
I'm not sure about the plain variant though. Technically the icon for the plain variant will work with either the icon prop or the current behavior where the icon is passed as the children. However a plain button should only be used with an icon, so we don't want to encourage use with text. However for the sake of consistency, I would be in favor of supporting either <Button variant="plain" aria-label="Action"><TimesIcon /></Button> or <Button variant="plain" aria-label="Action" icon={<TimesIcon />}></Button>, and we just make it clear in the documentation that you shouldn't pass regular text to the plain button. WDYT @tlabaj? Also not sure if this is a breaking change?