-
-
Notifications
You must be signed in to change notification settings - Fork 382
Fix icon color #3082
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
base: 2.x
Are you sure you want to change the base?
Fix icon color #3082
Conversation
dce6c8d
to
76d2253
Compare
76d2253
to
0751dad
Compare
This is also a massive BC break here, don't you think ? And losing the read-only side of thing .. DId you try simply setting the color on the icon ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the idea, but we need to find a better method...
And i'm not yet convinced of the usage... at least not in the one you provided.. maybe i'm not seeing something
} | ||
|
||
if ('fill' === $name) { | ||
$this->innerSvg = str_replace('fill="currentColor"', "fill=\"{$value}\"", $this->innerSvg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard BC break here.
} | ||
|
||
if ('stroke' === $name) { | ||
$this->innerSvg = str_replace('stroke="currentColor"', "stroke=\"{$value}\"", $this->innerSvg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep an only place we echo value and espace / encode them correctly (just below)
@smnandre Looking at this again this morning, I just found that this is possible with the color attribute and is even perfect in this case.
Perhaps the documentation should be modified accordingly, and mention that if you want to change the color, you can do it via CSS, as Iconify also mentions in its documentation. ![]() {{ ux_icon('flowbite:clock-outline', {class: 'size-4 text-gray-500'}) }} Or with this color attribute. And we ultimately avoid the current problem with the fill attribute. {{ ux_icon('flowbite:clock-outline', {class: 'size-4', color: 'gray'}) }} WDYT? |
Already noticed by another PR #2671 |
Very open if you want to open a PR on the documentation with clear and positive information about this (please do not only mention Tailwind that's the only thing I'm asking for haha) poke me if you need any guidance here :) |
If we use
fill
orstroke
attributes to update the color icon, as explained into https://symfony.com/bundles/ux-icons/current/index.html#default-attributes, nothing happens.Let's explore a default Boostrap SVG together: https://icons.getbootstrap.com/icons/person/
If we go into Bootstrap to grab that icon, it looks like this
UX-icon uses Iconify's icons and this icon is not completely the same https://github.com/iconify/icon-sets/blob/master/json/bi.json#L4472
Iconify returns the
path
tag for the bootsrap icon, this part is incremented with thefill
attribute or thestroke
, depend on the need.But if we add
fill
orstroke
to it will add these attributes to the svg tag and will not update the ones into thepath
tags, and that's the problem.With the modifications the
path
tag also follow the same rules