x64Bits / solid-icons

The simplest way to use icons in SolidJS
https://solid-icons.vercel.app
MIT License
284 stars 18 forks source link

color is wrong #44

Closed icode closed 8 months ago

icode commented 1 year ago

image

old version is right, latest is wrong, should set default color = curentColor

Schlumie commented 1 year ago

Some Icons are now black instead of white... in fill="currentColor" is missing there... workaround is to add fill="#xxx" on the solid iconelement...

densumesh commented 1 year ago

yup, worked for me. Had to set the fill: currentcolor prop on each solid-icon.

emdede commented 12 months ago

Without workaround of adding fill property image

With workaround applied image

I have a custom Icon component which takes IconType value and renders a Dynamic component which is an imported icon. Colors could be set by either passing the color prop or just by inheriting from currentColor set by a TW color class. This worked beautifully until the most recent changes. Now both ways of assigning an icon color are broken. This is extremely annoying as it forces me to micro-manage colors on a per-icon basis.

My code (shortened):

type IconProps = {
  type: IconTypes
  size?: IconSize
  color?: IconColor
  class?: string
}

export const IconType = {
  home: ImHome,
  domain: FiGlobe,
} as const

export type IconType = ValuesOf<typeof IconType>

export const IconColor = {
  white: '#ffffff',
  black: '#000000',
} as const

export type IconColor = ValuesOf<typeof IconColor>

export const IconSize = {
  sm: 16,
  md: 20,
} as const

export type IconSize = ValuesOf<typeof IconSize>

export const Icon: Component<IconProps> = (props) => {
  return (
    <Dynamic
      component={props.type}
      size={props.size ?? IconSize.md}
      color={props.color}
      class={props.class}
    />
  )
}

With newest changes I'd have to add a style property of fill which maps either color prop or currentColor and also add conditionals on a per-icon basis depending on if they show as intended or not (after visual confirmation). Any update on this in sight? I wanted to sit it out first, thinking it's a bug. But seeing that it's been like this for a few weeks already, I'd like to ask on current/future status.

I've checked the readme for updates. There, the color prop is still listed but it just isn't doing anything anymore.

x64Bits commented 12 months ago

Hello, I've been looking at this lately but I couldn't reproduce it, can you send a repro of this on github or codesandbox please?

emdede commented 12 months ago

@x64Bits It seems to be a specific issue with certain icon sets. https://stackblitz.com/edit/solid-ssr-vite-tj6b2f?file=src%2Froutes%2Findex.tsx

In my application I am using a wide variety of different source sets and the problem seems to be pretty wide-spread.

toondkn commented 11 months ago

Having the same issue. Almost all icons (if not all) are affected. All black instead of the current font color. Will investigate in the weekend.

Wantod commented 10 months ago

I personally have the problem with versions > 1.0.4. which is why I think there are so many downloads of this version. https://www.npmjs.com/package/solid-icons?activeTab=versions

emdede commented 10 months ago

@Wantod Yes, we also rolled back to 1.0.4

Just going through the commit history, the offending commit is pretty easy to find. I am not deep enough to be able to write a fix, obviously this commit was done for a reason. 1.0.4 works fine, so we just settled with that.

x64Bits commented 9 months ago

Hello everyone, one PR #51 was merge that address this issue, can anyone take a look it's the 1.0.12 versions display the icons correctly?

emdede commented 9 months ago

@x64Bits I've updated the example stackblitz from above with the new version and it does indeed seem to be working fine again.

x64Bits commented 8 months ago

Hi!, thank you very much for all the help, since version 1.1.0 even more problems with the color of the icons have been solved, I will close this issue, greetings!