Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<sl-icon color> inconsistencies #2382

Open
jamesarosen opened this issue Feb 20, 2025 · 4 comments
Open

<sl-icon color> inconsistencies #2382

jamesarosen opened this issue Feb 20, 2025 · 4 comments
Labels
bug Things that aren't working right in the library.

Comments

@jamesarosen
Copy link

jamesarosen commented Feb 20, 2025

Describe the bug

The <sl-icon> docs say

Icons inherit their color from the current text color. Thus, you can set the color property on the element or an ancestor to change the color.

All of the examples in the docs show style="color: #...;", not color="...".

<sl-icon> itself does accept color and sets it on the element, but that has no effect:

sl-icon with color

The React wrapper also accepts color?: string, but it does nothing.

To Reproduce

Create an <sl=icon color="#fedcba">

Additional information

Recommended actions:

  1. change the documentation to describe style="color: ..." rather than color="...", consistent with the examples and the working code
  2. non-breaking change: if <sl-icon> receives a color, emit a warning message
  3. breaking change: remove color from the accepted attributes for <sl-icon> and <SlIcon>
@jamesarosen jamesarosen added the bug Things that aren't working right in the library. label Feb 20, 2025
@KonnorRogers
Copy link
Collaborator

change the documentation to describe style="color: ..." rather than color="..."

I cant find an instance in the docs where we use color="" I only found style="color: ..."

breaking change: remove color from the accepted attributes for and

That doesnt come from us. It comes from @types/react https://github.com/DefinitelyTyped/DefinitelyTyped/blob/0d92bf97a2a32064302193f3305284bc925f0fee/types/react/v18/index.d.ts#L2950

which we use as the base for our custom element type so theres not a whole lot we can do to change it.

@jamesarosen
Copy link
Author

jamesarosen commented Feb 20, 2025

I cant find an instance in the docs where we use color=""

Right here:

Icons inherit their color from the current text color. Thus, you can set the color property on the <sl-icon> element or an ancestor to change the color.

Image

<sl-icon> doesn't have a color property:

$0
// <sl-icon name=​"exclamation-triangle" aria-hidden=​"true" library=​"default">​
'color' in $0
false
$0.color
// undefined

Image

It does accept a color attribute, but does nothing with it:

$0.setAttribute('color', '#102030')
$0
// <sl-icon name="exclamation-triangle" aria-hidden="true" library="default" color="#102030"></sl-icon>
window.getComputedStyle($0).color
// 'rgb(74, 144, 226)'

Image

I think the docs should say the color style property:

$0.style.color = '#102030'
$0
// <sl-icon name="exclamation-triangle" aria-hidden="true" library="default" color="#102030" style="color: rgb(16, 32, 48);"></sl-icon>
window.getComputedStyle($0).color
// 'rgb(16, 32, 48)'

@jamesarosen
Copy link
Author

Alas, it looks like you're right about color coming from @types/react. Since Shoelace components don't (in general) support color, perhaps the right solution is to add

& { color: never }

to the React wrappers.

@jamesarosen
Copy link
Author

FWIW, the reason I raised this is that I was converting from another icon component that does support color="..." as a React prop. That got passed to <sl-icon>, which threw it away. My type-checking would have caught that if <SlIcon> didn't declare a color prop that it doesn't support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

No branches or pull requests

2 participants