antfu-collective / icones

⚡️ Icon Explorer with Instant searching, powered by Iconify
https://icones.js.org
MIT License
4.94k stars 238 forks source link

feat: Default fill property set to path for React components #313

Closed osmangund closed 2 months ago

osmangund commented 2 months ago

For React component selections, I think there should be default fill props for path's fill property. Every time that property comes with "#eaeaea" value, and we have to change it manually. But as it's not the svg, we can't change it directly through component's props, so it's either going to be hardcoded or we'll define fill property to pass it as props on component. Since hardcoding is not the best practice, I assume this approach would be better for everyone. Thank you.

Suggested usage:

export function Heart(props, { fill }) {
  return (
    <svg
      xmlns="http://www.w3.org/2000/svg"
      width="1em"
      height="1em"
      viewBox="0 0 24 24"
      {...props}
    >
      <path
        fill={fill}
        d="m12 21l-1.45-1.3q-2.525-2.275-4.175-3.925T3.75 12.812T2.388 10.4T2 8.15Q2 5.8 3.575 4.225T7.5 2.65q1.3 0 2.475.55T12 4.75q.85-1 2.025-1.55t2.475-.55q2.35 0 3.925 1.575T22 8.15q0 1.15-.387 2.25t-1.363 2.412t-2.625 2.963T13.45 19.7z"
      ></path>
    </svg>
  )
}

Current usage:

export function Heart(props) {
  return (
    <svg
      xmlns="http://www.w3.org/2000/svg"
      width="1em"
      height="1em"
      viewBox="0 0 24 24"
      {...props}
    >
      <path
        fill="#eaeaea"
        d="m12 21l-1.45-1.3q-2.525-2.275-4.175-3.925T3.75 12.812T2.388 10.4T2 8.15Q2 5.8 3.575 4.225T7.5 2.65q1.3 0 2.475.55T12 4.75q.85-1 2.025-1.55t2.475-.55q2.35 0 3.925 1.575T22 8.15q0 1.15-.387 2.25t-1.363 2.412t-2.625 2.963T13.45 19.7z"
      ></path>
    </svg>
  )
}
cyberalien commented 2 months ago

React code samples use currentColor for fill, so your current usage example is incorrect.

Using currentColor makes it easy to style icon by adding style={{color: '#eaeaea'}}. No need to add extra properties.

Furthermore, using fill is incorrect because not all icons use fill. Many icons use stroke. Your solution would work only for a subset of icons. Using fill as prop for icons is a bad idea.