[SvgIcon] allow svg as a child

[siriwatknp]

Motivation

I have come across this situation many many times where I copy svg from 3rd parties, e.g. https://lucide.dev/icons or https://heroicons.com/, then I have to remove <svg> element all the time. I still need SvgIcon because it makes the theming consistent like color and fontSize props.

In another case using createSvgIcon in mui/mui-x#8940. I cannot set the default props that I want to use on the svg:

createSvgIcon(<path ... />)

// ❌ The icon does not work because I cannot pass the props to SvgIcon

Solution

The SvgIcon should work with svg as a child. It will merge the props defined in svg with the SvgIcon.

<SvgIcon>
  <svg fill="none" strokeWidth={1.5} stroke="currentColor">
    <path
      …
    />
  </svg>
</SvgIcon>

createSvgIcon(
  <svg fill="none" strokeWidth={1.5} stroke="currentColor">
    <path
      …
    />
  </svg>
)

Benefits

• better experience, you can just copy svg from anywhere and paste as a child to <SvgIcon>.
• able to customize the svg element when using createSvgIcon util.

---

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

• docs/data/material/components/icons/icons.md

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 765bad5

[siriwatknp]

@mui/core @oliviertassinari Any feedback or thought on this?

[DiegoAndai]

I like the benefits and solution, but I worry this might make the usage of SvgIcon more confusing:

• If the user doesn't provide the svg element, then <SvgIcon><path ... /></SvgIcon> turns into <svg><path ... /></svg>
• If the user provides the svg element, then <SvgIcon><svg><path ... /></svg></SvgIcon also turns into <svg><path ... /></svg>

That's what I think might be confusing, and won't make sense unless the user checks SvgIcon's implementation. What do you think about this?

[siriwatknp]

I like the benefits and solution, but I worry this might make the usage of SvgIcon more confusing:

• If the user doesn't provide the svg element, then <SvgIcon><path ... /></SvgIcon> turns into <svg><path ... /></svg>
• If the user provides the svg element, then <SvgIcon><svg><path ... /></svg></SvgIcon also turns into <svg><path ... /></svg>

That's what I think might be confusing, and won't make sense unless the user checks SvgIcon's implementation. What do you think about this?

Yes, that will be the trade-off with the DX. However, with clear documentation on how to use it, I don't think it will be confusing.

[DiegoAndai]

You're right 😊 might we add that documentation in this PR as well?

[siriwatknp]

You're right 😊 might we add that documentation in this PR as well?

Good point! updated the docs to mention <svg> as a child.

[DiegoAndai]

Looks good to me 🚀

[mnajdova]

Looks great, I faced this issue in the past too. I left a consideration for a test, the rest looks good

[mnajdova]

Let’s please add test for this

[mnajdova]

Aaah one thought, could this be considered a breaking change? We never supported svg before, right? If this is the case it should be fine

[scottopherson]

Aaah one thought, could this be considered a breaking change? We never supported svg before, right? If this is the case it should be fine

@mnajdova I think this was a breaking change, but perhaps not if y'all never explicitly supported supplying svg as direct child to SvgIcon.

Supplying svg as direct child to SvgIcon always worked fine for my team prior to us upgrading from 5.11.7 to 5.15.7; likely because it's perfectly valid to have nested svg elements:

[5.11.7 screenshot]

We've since had to add fill="currentColor" to these svg elements in order to maintain the previous behavior. Easy fix once we realized what was going on, though the diff had a lot of changed files!

Related: #40032 (comment)