canvas-kit icon indicating copy to clipboard operation
canvas-kit copied to clipboard

Icon button styling for icons is too restrictive

Open josephnle opened this issue 6 years ago • 1 comments

🐛 Bug Report

Current icon button styling method creates styles for the icon's colors at the IconButton component level. This causes issues when trying to color the icon for certain use cases. The SystemIcon component also already has methods to colors icons as part of its props API. Is there a reason why we can't use the SystemIcon coloring props for the IconButton component?

See line

To Reproduce

Steps to reproduce the behavior:

<IconButton
  variant={IconButton.Variants.Plain}
>
  <SystemIcon
    icon={calendarIcon}
    color={canvas.inputColors.focusBorder}
  />
</IconButton>

Expected Behavior

I expect to be able to color the inner SystemIcon as I need.

Actual Results

The icon is unable to be styled using the existing SystemIcon props API and is restricted to the prescribed colors set in the IconButton styles.

josephnle avatar Feb 26 '20 00:02 josephnle

Actually I think allowing the user to modify hover color like this is not a good idea.

      <IconButton
        aria-label="Activity Stream"
        title="Activity Stream"
        variant={IconButton.Variant.Circle}
        size={IconButton.Size.Medium}
      >
        <SystemIcon
          icon={activityStreamIcon}
          color={canvas.colors.blueberry400}
          colorHover={canvas.colors.blueberry400}
        />
      </IconButton>

hover button Hover from button will be applied to icon, and will override initial color. However when we hover on the icon, only in that case we would apply hover style to it, looks a bit odd.

Instead, I think there should be hover style only on IconButton. I suggest removing children prop from IconButton to avoid confusion.

However in Learning project, we need IconButton with blueberry icon.

Solutions proposed:

  • provide props to IconButton: (colorHover, colorIconHover, color, colorIcon)
  • we could add another Variant which would work for our case.

alexandrzavalii avatar Apr 27 '20 18:04 alexandrzavalii

Closing in favor of a more restrictive color API.

jaclynjessup avatar Sep 08 '22 21:09 jaclynjessup