Sorry it took me a while to review this. The patch applies, but it's actually changing the color for several icons with different level of visual implication.
In #3045216: Asset paths pointing to core/misc folder assume Claro is installed in core/themes the images/core directory was created for containing copies of core icons used by Claro. These copies existed because Claro was a contrib module and it's location relative to /core was not consistent.
images/core has a readme that states
Icons in this folder are copied from Drupal core. This folder with its content
should be removed before moving Claro to Drupal coreClaro is now in Drupal core, so this change can happen.
| Comment | File | Size | Author |
|---|---|---|---|
| #47 | 3133823-47.patch | 19.73 KB | gauravvvv |
| #44 | interdiff_43-44.txt | 15.9 KB | akram khan |
| #44 | 3133823-44.patch | 20.25 KB | akram khan |
| #43 | interdiff_39-43.txt | 5.97 KB | akram khan |
| #43 | 3133823-43.patch | 29.75 KB | akram khan |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
nitesh624Comment #3
nitesh624Comment #4
nitesh624Comment #5
kristen polI'm confused. The issue summary says:
Icons in this folder are copied from Drupal core.If I search for these files, they are in
seven, e.g.or
stable, e.g.Shouldn't this issue be a general refactor to move icons from themes to
core/misc/iconsand then use them from there? Except maybestableandstable9since we might not be able to touch those from what I remember from another issue.Comment #6
nitesh624hi @Kristen Pol thanks for your reply . what will be the next action to do with this issue. can you pls specify?
patch 3133823-3.patch is doing the same things exactly
Comment #7
bnjmnmRe: #5 it looks like the contents of the
/ccccccdirectory are not copied from core. This was a mistake and should also be addressed in this issue. Those images are unique to Claro and can be moved to a different directory within Claro (images/icons should work). The css refactored to use those. The current patch CSS has several references to those images images as if they exist in/core/misc/icons/cccccc/but do not, so that definitely needs to be fixed.I believe everything else has a copy in /misc/icons but each one should be verified.
No, this was something that was planned to happen when Claro was added to core, but was missed. It's not clear if this is something that should even happen in other themes, but those can be done in separate issues if there's a desire to do so.
Comment #8
nitesh624move the "cccccc" directory under images/core to images/icons
Comment #9
kristen polThanks for the information @bnjmnm and the updated patch @nitesh624.
One reason why I thought there might be some duplication of icons across themes is that I diff'ed a couple svg files and they were identical. But, I only did a couple. And, maybe it's fine they are identical since they could diverge in the future and so then should be kept separately.
For reference, I grabbed the locations of all the svg files noted in the patch. Someone could go and diff all these to see if any are identical. I might be able to later but can't right now.
Side note that I know you can pass in a
-namearg tofindbut I find typing the above pattern faster ;)Comment #10
nitesh624Thanks @Kristen Pol i will do this to check for duplication
Comment #11
nitesh624Comment #12
kristen polThanks. Just to be clear, even if the same exact icon is used in multiple themes (as separate files), I don't know if the theme maintainers would want any moved into the non-theme directory. But I think it's still useful to know what is where and which are identical.
Comment #13
nitesh624Similar images
required.svg
./core/misc/icons/ee0000/required.svg ./core/themes/stable/images/core/icons/ee0000/required.svgex.svg
questionmark-disc.svg
./core/misc/icons/787878/questionmark-disc.svg ./core/themes/stable/images/core/icons/787878/questionmark-disc.svg./core/misc/icons/ffffff/questionmark-disc.svg ./core/themes/stable/images/core/icons/ffffff/questionmark-disc.svg./core/misc/icons/bebebe/questionmark-disc.svg ./core/themes/stable/images/core/icons/bebebe/questionmark-disc.svg./core/misc/icons/000000/questionmark-disc.svg ./core/themes/stable/images/core/icons/000000/questionmark-disc.svgkey.svg
./core/misc/icons/787878/key.svg ./core/themes/stable/images/core/icons/787878/key.svgcog.svg
error.svg
warning.svg
check.svg
./core/misc/icons/73b355/check.svg ./core/themes/stable/images/core/icons/73b355/check.svgd8-logo.svg
clock.svg
./core/themes/claro/images/core/cccccc/clock.svg ./core/themes/seven/images/icons/cccccc/clock.svgserver.svg
php-logo.svg
database.svg
caret-down.svg
Different images
required.svg
ex.svg
questionmark-disc.svg
key.svg
cog.svg
error.svg
warning.svg
check.svg
d8-logo.svg
./core/themes/claro/images/core/cccccc/d8-logo.svg ./core/themes/seven/images/icons/cccccc/d8-logo.svgclock.svg
server.svg
php-logo.svg
./core/themes/claro/images/core/cccccc/php-logo.svg ./core/themes/seven/images/icons/cccccc/php-logo.svgdatabase.svg
./core/themes/claro/images/core/cccccc/database.svg ./core/themes/seven/images/icons/cccccc/database.svgcaret-down.svg
./core/themes/stable/images/core/icons/333333/caret-down.svg ./core/themes/claro/images/core/333333/caret-down.svgComment #14
nitesh624Comment #15
kristen pol@nitesh624 Thanks for checking.
Sorry I wasn't clear. I didn't mean to put the diffs in here but rather just to note which are the same and which are different. Might be good to edit the comment above to simplify it so it's easier to read.
Comment #16
nitesh624ok @Kristen Pol i will do this
Comment #17
kristen pol@nitesh624 That is helpful, thanks. Addressing my questions from before:
1) Should any images be moved from
clarotocore/misc/icons/?You have that these non-stable images as equivalent:
So, does it make sense to move
clock.svgand/orserver.svgtocore/misc/icons/and refer to them fromclaroandseven? Or, instead, is it better to let them stay where they are so the themes can replace these images later if they want?Since there are only two non-stable files that are equivalent, IMO these are fine left in place.
2) Should any use the equivalent existing
core/misc/icons/?There don't appear to be any candidates for this. Other than the two noted above, the icons that are equivalent are for
stableand, from what I understand,stablecan't be modified.3) Reading #7 again and reviewing the equivalent files from #13, I agree that it's fine to just rename the directory like suggested in #7.
4) Checking the patch in #8, I don't think that is what is happening. Per #7, the path can go from
images/coretoimages/icons.5) I poked around the
imagesdirectory and am now wondering about:a)
images/core/stable/views_ui/sprites.png- to be replaced, removed, moved?b)
images/classy- to be replaced, removed, moved?c) IMO, that directory is a bit messy, e.g. all the
ui-icons-*icons at the top level.6) That's all I have energy for right now. Leaving "Needs review" for others to chime in if they are able.
Comment #18
kristen pol@bnjmnm Am curious if you have any thoughts regarding #17.5.
Comment #19
kristen polI think we need some proper guidance on this one. The images seem to be in a messy state and it's hard to know how to proceed on this.
Comment #22
sakthivel m commented@ Needs Re-rolls
Comment #23
sakthivel m commented#23 Please review the patch
Comment #25
ckrinaComment #29
rpayanmRerolled
Comment #30
rinku jacob 13 commentedpatch#23 failed to apply for 9.5.x version
Comment #31
ameymudras commentedRerolling the patch as the above patch doesn't apply correctly
Comment #33
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #34
gauravvvv commentedPatch #31, no more applies to
10.1.x. because some files doesn't exist in 10.1.x. Adding patch for 10.1.x. Please reviewComment #35
gauravvvv commentedComment #36
smustgrave commentedThink patch #34 is missing some changes. The patch is much smaller then #23.
Think somethings were removed in #31
From what I can tell the new images aren't there.
Comment #37
gauravvvv commentedI have added the missing SVG image. please review
Comment #38
santosh_verma commentedTested the Patch #37 and getting error while patch applying
error: while searching for:
*/
.system-status-counter {
--system-status-counter-status-icon: #e6e4df;
--system-status-counter-status-icon-error: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' width='16' height='16' fill='%23dc2323'%3e%3cpath d='M8.002 1c-3.868 0-7.002 3.134-7.002 7s3.134 7 7.002 7c3.865 0 7-3.134 7-7s-3.135-7-7-7zm4.025 9.284c.062.063.1.149.1.239 0 .091-.037.177-.1.24l-1.262 1.262c-.064.062-.15.1-.24.1s-.176-.036-.24-.1l-2.283-2.283-2.286 2.283c-.064.062-.15.1-.24.1s-.176-.036-.24-.1l-1.261-1.262c-.063-.062-.1-.148-.1-.24 0-.088.036-.176.1-.238l2.283-2.285-2.283-2.284c-.063-.064-.1-.15-.1-.24s.036-.176.1-.24l1.262-1.262c.063-.063.149-.1.24-.1.089 0 .176.036.24.1l2.285 2.284 2.283-2.284c.064-.063.15-.1.24-.1s.176.036.24.1l1.262 1.262c.062.063.1.149.1.24 0 .089-.037.176-.1.24l-2.283 2.284 2.283 2.284z'/%3e%3c/svg%3e");
--system-status-counter-status-icon-warning: url("data:image/svg+xml,%3csvg fill='%23e29700' height='16' width='16' xmlns='http://www.w3.org/2000/svg'%3e%3cpath d='m14.66 12.316-5.316-10.633c-.738-1.476-1.946-1.476-2.685 0l-5.317 10.633c-.738 1.477.008 2.684 1.658 2.684h10.002c1.65 0 2.396-1.207 1.658-2.684zm-7.66-8.316h2.002v5h-2.002zm2.252 8.615c0 .344-.281.625-.625.625h-1.25c-.345 0-.626-.281-.626-.625v-1.239c0-.344.281-.625.626-.625h1.25c.344 0 .625.281.625.625z'/%3e%3c/svg%3e");
--system-status-counter-status-icon-checked: url("data:image/svg+xml,%3csvg fill='%2373b355' height='16' width='16' xmlns='http://www.w3.org/2000/svg'%3e%3cpath d='m6.464 13.676c-.194.194-.513.194-.707 0l-4.96-4.955c-.194-.193-.194-.513 0-.707l1.405-1.407c.194-.195.512-.195.707 0l2.849 2.848c.194.193.513.193.707 0l6.629-6.626c.195-.194.514-.194.707 0l1.404 1.404c.193.194.193.513 0 .707z'/%3e%3c/svg%3e");
display: inline-block;
overflow-y: hidden;
error: patch failed: core/themes/claro/css/components/system-status-counter.css:10
error: core/themes/claro/css/components/system-status-counter.css: patch does not apply
Checking patch core/themes/claro/css/components/system-status-counter.pcss.css...
error: while searching for:
*/
.system-status-counter {
--system-status-counter-status-icon: #e6e4df;
--system-status-counter-status-icon-error: url(../../../../misc/icons/dc2323/error.svg);
--system-status-counter-status-icon-warning: url(../../images/core/e29700/warning.svg);
--system-status-counter-status-icon-checked: url(../../images/core/73b355/check.svg);
display: inline-block;
overflow-y: hidden;
error: patch failed: core/themes/claro/css/components/system-status-counter.pcss.css:4
error: core/themes/claro/css/components/system-status-counter.pcss.css: patch does not apply
Comment #39
gauravvvv commentedfixed build error. attached interdiff for same.
Comment #40
smustgrave commentedMissing changes have been added back.
Comment #41
ckrinaSorry it took me a while to review this. The patch applies, but it's actually changing the color for several icons with different level of visual implication.
This shouldn't be changing the icon color from white(#fff) to
#CCC. So it should land on the existing folder/ffffff.Same, this shouldn't be changing the icon color from white to
#CCC. So it should land on the existing folder/ffffff.Same, this shouldn't be changing the icon color from white to
#CCC. So it should land on the existing folder/ffffff.Same, this shouldn't be changing the icon color from white to
#CCC. So it should land on the existing folder/ffffff.Why is this changing the color from
#dc2323to#e32700? I understand this is to reuse the default core icon, but it's actually changing the red used in Claro. Meaning it's changing the designs. Both icons still exist in core, so please keep the same color. Another question is way we keep the same icon in core with similar reds, but that should be solved in another issue :)Comment #42
ckrinaI forgot to reference a related issue for Claro related to icons: #3269881: Update icon colors with the new color scale
Comment #43
akram khanadded patch and address #41
Comment #44
akram khantry to fix CCF #43
Comment #46
robcarrThank you for all the work on this issue. I've tried most of the recent patches against D10.1-beta1, and cannot get them to apply.
Comment #47
gauravvvv commentedI have provided patch for 11.x, addressed #41. please review
Comment #48
smustgrave commentedFrom what I can tell the images are still landing in cccccc
Comment #50
gauravvvv commentedAddressed #48, please review
Comment #51
bnjmnmWhen I filed this issue in 2020...
Given that it is 2+ years since Claro graduated from experimental status, I'm not sure the benefits of of moving the assets + references outweigh the potential disruption. Testing this manually requires time and it is a lengthy process without tests to confirm things are OK. Something unwanted could slip through, and custom modules/themes may be referencing some of these assets. Had this happened while Claro was still experimental I'd be all for the change. While it should have occurred when Claro was added to core on October 4, 2019, that doesn't necessarily mean it should happen now. I'm setting this to Closed (outdated), but I don't intend this to be a stern "final word". If anyone here believes the benefits of doing this outweigh the risks then document that and re-open.
Alternatively, get some quick issue credit and open an issue that simply removes this from the readme: