LeadingIcon - passing svg prop as string results in unexpected SVG styling

[ByronDWall]

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:

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

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

Screenshots

Screen.Recording.2024-04-11.at.1.36.17.PM.mov

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:

1. remove the *:not([fill='none']) selector from SVG's built by the InlineSVG component for SVG's rendered by LeadingIcon

2. accept a ReactComponent instead of a string for the LeadingIcon svg prop.

[ddouglasz]

Hey Byron,
Thanks for reporting. I can also see this from my end too.
We will take a look at a bit further and let you know how we would like to proceed.

[emmenko]

I would suggest to not use the InlineSVG implementation for the LeadingIcon component if you don't want the default icons behavior.

On another note, if you want to preserve the SVG as-is, why not loading the SVG directly as a React component? The MC tooling already allows that if the file name ends with .react.svg.
Then you just need a "container" to render the SVG component within certain constraints.

Thanks

[ByronDWall]

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]

@emmenko I'm just fine with passing a react component, the api/props and how they are handled in LeadingIcon were defined for us by the Shield team when we initially implemented the component, and specified that the svg prop should be a string.

I can put in a PR to implement the change if that's alright with everyone.

[emmenko]

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).

[CarlosCortizasCT]

I think the idea behind the svg property was not to use it for custom icons but for use cases where you need to use an inline SVG (eg: an SVG string stored in a database).

We should be using the icon property for most of the use cases.

[ByronDWall]

@CarlosCortizasCT the issue with that is that the icon prop has styling that is meant specifically for ui-kit icons (most noticeably the padding). The svg prop has styling that is more suited to displaying "custom" svg's, and there is no way to apply that styling to a react component if you pass it using the icon prop.

As an example, you can see the icon in this card on the welcome page as it is currently:

And how it is displayed when being passed to the icon prop:

[CarlosCortizasCT]

Hi @ByronDWall

I'm sorry but I'm struggling to understand this sentence:

the issue with that is that the icon prop has styling that is meant specifically for ui-kit icons (most noticeably the padding).

The way I see it, all icons should be rendered the same not matter whether they are part of ui-kit or not. We (commercetools) should aim for visual consistency and we would breaking that if different icons have, for instance, different paddings.

Perhaps I'm mistaken here and we can maybe have a meeting with @FilPob to discuss this further and better understand, at least from my side, the reasoning behind this requirement.

Would that work for you?

[ByronDWall]

hey @CarlosCortizasCT that works for me. I am happy to discuss this with you and Filip. Our team is having our onsite meeting this week, so my availability is very limited until Friday.

I'm sure we can come to a reasonable decision either asynchronously during the week or in a quick meeting Friday. Thanks!

[CarlosCortizasCT]

Hi @ByronDWall

@FilPob and I had a discussion about this issue and know I understand better what the problem is.
I didn't fully understand the requirements of the LeadingIcon when it was analyzed so I missed an important part. Sorry about that.

Now that I have a better understanding though, I'm hesitant about the LeadingIcon supporting custom SVGs.
As stated in its documentation (driven by Filip):

The leading icon is a an eye-catching visual element that should be used when 
an additional visual prominence is needed for a content section in the UI. 
The different colours in combination with the icons can be utilised to create
certain categorisation of the elements in the UI.

This component can be used wherever it is necessary to display a themed icon.

The way I understand this component is to have a themed icon in order to increase visual attention by using prominent colors but the way custom icons are being used does not fall into path as they need to have their own colors and a transparent background.

The only parts both use cases share is the size and the border (although the latter is only try for one LeadingIcon variant).

All that leads me to think whether having custom SVGs actually make sense in the LeadingIcon or if we should have a different component for wrapping custom icons.

I think the idea with custom icons is that we want to allow any kind of icons but we want them to share the sizes range and the border so I believe it would make more sense to have something like a CustomIcon component which just apply those restrictions and accepts both an icon and an inline SVGs without setting any styles on them.

@FilPob @ByronDWall what do you about this proposal?

[FilPob]

@CarlosCortizasCT thanks for the explanation. I agree with the approach of the new component. Just two things to clarify

1. The new component would not have any padding right? Cause that's necessary for the use cases with the inline svg. However if you would pass our icon instead of the inline svg, it could look weird again without any padding.

CustomIcon component which just apply those restrictions and accepts both an icon and an inline SVGs

Should the new Customicon component really accept also our UI Kit icons instead of the inline svg? Then it wouldn't really be "custom" anymore right? Wdyt?

[CarlosCortizasCT]

@CarlosCortizasCT thanks for the explanation. I agree with the approach of the new component. Just two things to clarify

1. The new component would not have any padding right? Cause that's necessary for the use cases with the inline svg. However if you would pass our icon instead of the inline svg, it could look weird again without any padding.

CustomIcon component which just apply those restrictions and accepts both an icon and an inline SVGs

Should the new Customicon component really accept also our UI Kit icons instead of the inline svg? Then it wouldn't really be "custom" anymore right? Wdyt?

1. I'm not sure if understand your question. On the one hand, yet the component would not have any padding as that's what I understood from our previous conversation. If a consumer needs a padding, they can include it in the icon itself. If you think we should allow only a subset of paddings, then we can include a property for consumers to select which paddings to use.

2. This new proposed CustomIcon is not intended to be used with our icons. Do you have any use case in mind where that would be required?

This would be the components API:

type TCustomIconProps = {
  size?: 'small' | 'medium' | 'big' | 'scale'; // <--- Same as our icons
};

The component will render the provided icon with the selected size and with a rounded border with no paddings.

I think what we're trying to cover is for icons that are required in specific use cases, to have a consistent size.

To be honest, the border thing should also be optional from my point of view because it will depend on the specific icon used, but maybe you have more context here and it's better to always use it.

[FilPob]

okay then I misunderstand it before. I agree that the CustomIcon should not be used with our icons.

+1 to making the border optional
+1 for no padding

regarding the size: For Byron's use case, we would need that CustomIcon to be 48px. (if the designs from some weeks ago are still valid) That's why the LeadingIcon in size: 30 was intended to be used. Would this mean he should use the size: scale to set it manually to 48px? Cause the other icon sizes we have small: 12px, medium: 16px, big: 24px wont be enough for Byron's use case

[CarlosCortizasCT]

Thanks @FilPob

That depends on what you think the custom icons should look like in terms of the size.

If we want for them to have the same size options as the LeadingIcon, that's something we can do.

[FilPob]

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!

[CarlosCortizasCT]

Please @ByronDWall, can you confirm this new component would be enough for your use case?

[ByronDWall]

@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?

[CarlosCortizasCT]

@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.

[ByronDWall]

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

[CarlosCortizasCT]

@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.

[ByronDWall]

works for me, thanks!

[CarlosCortizasCT]

@ByronDWall would it be ok to close this issue since we already implemented the new CustomIcon component? 🤔

[ByronDWall]

@CarlosCortizasCT I think so, thanks!