De-duplicate warning.svg icons
Categories
(Toolkit :: Themes, task, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox92 | --- | verified |
People
(Reporter: ntim, Assigned: mhowell)
References
(Blocks 1 open bug)
Details
(Whiteboard: [proton-icons] [proton-cleanups])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
toolkit/themes/shared/icons/warning.svg
browser/themes/shared/warning.svg
browser/themes/shared/controlcenter/warning.svg
All these 3 are more-or-less the same icon.
The browser ones have a configurable stroke, where toolkit one is a cut out.
Reporter | ||
Comment 1•4 years ago
|
||
DevTools has its own versions too:
devtools/client/themes/images/alert-small.svg
devtools/client/themes/images/alert-tiny.svg
devtools/client/themes/images/alert.svg
Also, it's worth mentioning browser/themes/shared/warning.svg is used as app menu badge, if a different icon is still needed there, worth renaming it to badge-warning.svg
or whatever convention is used for the update badge.
Reporter | ||
Comment 2•4 years ago
|
||
I recommend keeping only the cut out version (toolkit/themes/shared/icons/warning.svg), since that's what proton is going to be using.
Reporter | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
browser/themes/shared/controlcenter/warning.svg
has already been removed in bug 1707086.
The devtools clones were intentionally created in bug 1699858; Proton didn't apply to devtools, but we updated some of the shared icons that devtools was using, so copies of those were made to prevent having two different icon styles. I think that rationale stills applies, so I don't think those could be easily deduplicated at the moment.
That leaves toolkit/themes/shared/icons/warning.svg
and browser/themes/shared/warning.svg
, which do appear to be identical except that the one in browser/ has colors in it and the one in toolkit/ does not; not an especially meaningful difference. Sam, do you know of a particular reason why both of these are still here, or can one just be removed?
Comment 4•3 years ago
|
||
(In reply to Molly Howell (she/her) [:mhowell] from comment #3)
That leaves
toolkit/themes/shared/icons/warning.svg
andbrowser/themes/shared/warning.svg
, which do appear to be identical except that the one in browser/ has colors in it and the one in toolkit/ does not; not an especially meaningful difference. Sam, do you know of a particular reason why both of these are still here, or can one just be removed?
I don't know a specific reason. Ideally we always want the fill color to come from the CSS with these icons, but the difference between the browser and toolkit variations is that the browser one has a fallback color hardcoded in there. Its possible that is there because it was difficult to target with a CSS rule in some of its uses?
So, ideally we'd remove browser/themes/shared/warning.svg and update all references to it with toolkit/themes/shared/warning.svg, but we have to be super careful that each of those references provides an appropriate context-fill color to avoid regressions.
Assignee | ||
Comment 5•3 years ago
|
||
Okay, thanks for the info. I think I'll give it a go, and just be very careful that there's always a color present, like you said.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
The only difference between the icon that was removed and the one kept is that
the removed one has a default fill color in the SVG. This meant everywhere the
icon was replaced, we have to make sure that a fill color is defined in CSS.
In a few cases, that necessitated adding a new class. In a few others, colors
were already being defined for the icon, so there was no need to add any here.
Comment 8•3 years ago
|
||
bugherder |
Comment 10•3 years ago
|
||
Verified using Nightly 92.0a1 (20210729093615) on Windows, MacOS and Ubuntu.
Description
•