-
-
Notifications
You must be signed in to change notification settings - Fork 299
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
[ux-icons] fill attribute override #1803
Comments
In the meanwhile, not sure, but maybe one of |
Hello and thank your for your suggestion, I think I don't understand where you image putting fill revert or revert layer? Because whatever you put in svg tag as a fill value, it will be overridden by currentColor from config. I also edited my post to add another interesting case, points 6, 7 & 8. Not sur if this behavior is intended, I feel like not but I may miss a usecase; I may change this, and I suppose the attributes are computed in this method, but as I'm not good enough to do it fast, I can do this next week only. But I wonder here some code should check if attribute already exists, then not override it from conf (but do it from forced attribute, like |
Oh sorry i have not been explicit enough :) This is a bug, and i'll try to fix it as soon as possible, sorry for that :) Until then, i think you may prevent the currentColor attribute to have an effect if you target the desired SVG ? svg.brand-icon {
/** one of those lines _may_ work */
fill: revert;
fill: revert-layer;
} Probably with some !important or scope there i guess |
After some thinking.. i'm a bit hesitant there :| If i defined "width: 24px" in the configuration, i expect my icons to be 24px, whatever the original attribute was. And it's how the documentation explain it to work (in theory no attribute should be on SVG icons). So maybe what is missing is some flag to either force override or keep original (what i'd find more usefull in practice).... ... but i'm not sure we should go there for now. What do you think ? |
Hello @smnandre, That's a good point! And I'm partially sold to it. Nevertheless, let me explain some counter arguments to it 😄
<svg viewBox="0 0 240 267" xmlns="http://www.w3.org/2000/svg">
<path
fill="#FF7324"
d="M237.969 58.1199C168.331 76.284 106.561 108.419 56.9547 162.626C69.4662 176.402 83.775 180.628 100.754
183.852C137.221 145.596 181.923 121.297 232.259 106.546C244.627 102.916 238.387 69.4299 237.969 58.1199Z"
/>
<path
fill="#FF7324"
d="M54.5035 60.3423C2.38617 90.9409 -15.4728 158.733 14.614 211.728C30.1498 239.089 55.4372 257.166 83.4303
264.036C167.897 284.728 238.485 193.114 235.499 112.728C187.509 127.22 145.8 150.942 111.675 185.803C211.933
203.374 121.567 275.268 57.3366 231.12C11.2647 199.452 5.88849 140.034 32.0682 95.5783C48.1521 68.2625 70.6671
57.8372 97.822 46.0573C82.9969 47.5999 68.2419 52.2729 54.5035 60.3423Z"
/>
<path
fill="#FF7324"
d="M237.618 1.00075C145.086 30.0503 45.0931 56.3388 39.3281 117.471C38.047 126.555 39.9305 136.468 46.1479
147.415C48.1397 150.918 50.1729 154.052 52.2571 156.875C99.7789 102.133 160.504 69.6353 232.647
50.9333C245.123 47.6991 238.039 12.3075 237.618 1.00075Z"
/>
</svg> Note that is a brand logo from the challonge.com website (I'm not related to it), that I "minimized" to put in my collection. ViewPort is mandatory, height & width aren't. Not how I have to fill with Orange in each path for now 😉 . Point is that you can use it as a favicon, brand logo on your header, background image or action pictogram with this base. That's exactly why SVGs are powerful. I put this argument as the last one because I will not by myself make every svgs creator removes their height & width properties, so it's basically just some wishfull thinking. I'm pretty happy with my arguments and I managed to switch back my opinion about this configuration point, so I'm please to share them with you. 😊 |
I have plenty of those :) data-iconset="lucide" class="icon-lucide" hidden style="--foo: #45261" stroke="none"
Let's keep that in mind for the future yes
I worked on this bundle to not be forced to pass arguments :) I want to use configuration and not using hard-coded values dispatched in multiple templates.
Yes but it's not the goal of the feature :)
Yes that would be the idea... let's keep that in mind, that may rejoin the class idea
I can change them in CSS / pass arguments in the rare case the 24/24 model will not work
Oh there are many cases ;) And currentColor has the advantage to be easy to change in CSS. After..... you also can change the configuration and not inject currentColor to all your icons :)
I think they would ... and if they would not, they would set something else instead :) And again, nothing is required there ;)
See my previous answer... it's not to give a size but a ratio :)
How do you handle darkmode ? Negative rendering ? In reality there are many usages / needs out there... We set as default configuration this value, because we think it's a good practice / DX.. and it can be removed :) So i'm very happy to have this very interesting discussion with you..... .. let's see what other think :) |
Poke #2156 |
Hello,
I continue to play with ux-icons, and there is a weird behavior:
fill="#fff"
attribute,<div style="color: red;">{{ ux_icon('mysvg') }}</div>
,<div style="color: red;">{{ ux_icon('mysvg', {fill: '#00f'}) }}</div>
,When you render this, your svg is filled by red, not by your white #fff. Thing is that the ux_icon component put the default
fill="currentColor"
attribute even if there is already a fill attribute in the file.Maybe it is not intended, or counter-intuitive, and the function should check if fill exists before writing one? Because is a given project, there is some icons that you want to be color swappable, and that can be a majority that deserves a default
fill="currentColor"
config for it, but also have few icons that must stay in a given color and not move from it (at least not as easily as justcolor: whatever
). For example brand icons, action buttons, ...Current workaround is to put the fill attribute inside the svg tag, for example on a path tag or wrap the tags with a g tag. Or to give the color control to CSS 😄
The text was updated successfully, but these errors were encountered: