ui-kit: `LeadingIcon` - passing `svg` prop as `string` results in unexpected SVG styling

Describe the bug The LeadingIcon component API currently accepts an svg prop, which is currently a string that is rendered internally by the InlineSVG component.

The intention behind allowing for an svg prop as well as an icon prop is so that the LeadingIcon can display “custom” SVG’s.

The InlineSVG component sets all fill values within the SVG to fill: inherit by default in this style method which means that all elements of the SVG are rendered in a single color.

This means that custom SVG’s displayed by LeadingIcon will always have a single color, which leads to unexpected results when passing in SVG’s that have multiple fill colors.

To Reproduce Steps to reproduce the behavior:

Import LeadingIcon into a file
Implement an svg that has multiple fill values as a string
Pass string to LeadingIcon component via svg prop
LeadingIcon displays an svg with only one fill color

Expected behavior SVG’s passed to LeadingIcon are displayed with all declared fill colors.

Screenshots

https://github.com/commercetools/ui-kit/assets/87667330/8dd8f957-e9c6-4017-a388-03a88cdf9320

Additional context Being able to display custom SVG’s as-expected in LeadingIcon is one of the main use-cases our team needs to be able to achieve with this component, so I would like to suggest a couple of solutions that the first contact team could implement relatively quickly:

remove the *:not([fill='none']) selector from SVG’s built by the InlineSVG component for SVG’s rendered by LeadingIcon
accept a ReactComponent instead of a string for the LeadingIcon svg prop.

About this issue

Original URL
State: open
Created 3 months ago
Comments: 21 (18 by maintainers)

Most upvoted comments

@FilPob @CarlosCortizasCT Hey, sorry I missed this conversation until now. I think that having a separate CustomIcon component from LeadingIcon makes a lot of sense here.

Overall, I think the proposed component api makes a lot of sense, I just want to recap so I am sure I’m not missing anything:

accepts a ReactNode svg that is not a ui-kit Icon

same sizing props as LeadingIcon

optional border

no padding

transparent background

Does this sound right?

Yes, those would be the requirements.

We can also discuss if we would need to support inline SVGs on top of ReactNode, but that’s something we can do as a follow-up maybe.

CarlosCortizasCT on Apr 22, 2024

works for me, thanks!

ByronDWall on Apr 22, 2024

@CarlosCortizasCT would you like to remove support for inline SVG’s from LeadingIcon as well?

That’s something we can also do as a follow-up, but that’s technically a breaking change so I wouldn’t get into that for now.

CarlosCortizasCT on Apr 22, 2024

Ah okay right we could reuse the sizes of the leadingIcon Yes then I dont see any more issues. Makes sense now. Thanks for the patience!

FilPob on Apr 22, 2024

I just want to make sure that we don’t change anything related to InlineSVG. The use case for that is specifically for dynamically rendering any SVG for Custom Application icons. Which is why it behaves like the normal icons (for styling).

I honestly don’t know much about the LeadingIcon requirements as I wasn’t involved in the planning (which is totally fine).

emmenko on Apr 12, 2024

Hey @ddouglasz, thanks for responding. I was the person who originally implemented the LeadingIcon, so I am comfortable with going in and making necessary changes for this.

One thing I’d like to point out is that expecting an SVG as a string is a bit of an awkward pattern, and goes against current best practices for handling SVG assets in merchant center applications (which is to build them as react components with the *.react.svg file type).

Perhaps it would make sense for the svg prop to accept either a string or a ReactComponent, and update how InlineSVG handles fill when it is called by LeadingIcon?

ByronDWall on Apr 12, 2024