When building a composer installed Claro theme (running yarn install and yarn build:css) there are missing icons. This happens with all icons loaded from the core/misc folder, for example required.svg.
claro/css/src/components/form.css
sets
.form-item__label.form-required::after,
.fieldset__label.form-required::after {
...
background-image: url(../../../../misc/icons/ee0000/required.svg);
...
}
This results in a 'resource not found' console error looking for the asset at /themes/contrib/misc/icons/ee0000/required.svg
To reproduce this issue:
composer require drupal/claro
cd (docroot)/themes/contrib/claro
yarn install
yarn build:css
Go to your Drupal site, with Claro set as admin theme. Navigate to an admin form with required fields. No required markers appear, console has the error.
Comment | File | Size | Author |
---|---|---|---|
#23 | 3045216-23.patch | 20.9 KB | bnjmnm |
| |||
#23 | interdiff_19-23.txt | 921 bytes | bnjmnm |
#20 | claro-path_to_images-3045216-binaryfix.patch | 20 KB | bnjmnm |
| |||
#19 | interdiff.txt | 20.32 KB | lauriii |
#19 | claro-path_to_images-3045216-19.patch | 17.99 KB | lauriii |
Comments
Comment #2
lauriiiComment #3
ccasals CreditAttribution: ccasals at Phase2 commentedAh, that makes a lot of sense!
So the way that we are making this work temporarily, is to use composer installer paths. Which if Claro is never supposed to really "live" as a contrib theme, might be a temporary good solution.
However, the path is still not correct. If Claro is in core/themes/ directory, the console error becomes "failed to load resource" for path /core/themes/misc/icons/ee0000/required.svg .
Comment #4
ccasals CreditAttribution: ccasals at Phase2 commentedThese are the places I could find pointing to "misc". These updated paths should work if claro is in the core/themes directory.
Comment #5
ccasals CreditAttribution: ccasals at Phase2 commentedSince the intended final destination for claro is core/themes, this is ready for review!
Comment #6
lauriiiAny thoughts if we would copy all core assets temporarily to Claro so that Claro could be installed in any folder? The assets in the core don't change all that often so I don't think this would be a big problem.
Comment #7
pranav.aeer CreditAttribution: pranav.aeer commentedAs the final path for claro will be core/themes, I've just applied patch provided on #4 and tested, it works for me.
Comment #8
pranav.aeer CreditAttribution: pranav.aeer commentedComment #9
lauriiiI think it would be great if we could come up with a solution that would (temporarily) allow installing the theme in any supported folder.
Comment #10
pranav.aeer CreditAttribution: pranav.aeer commentedI have updated the paths of the places where I could find pointing to "misc" as provided in #4 comment, So please check the attached path, I think this may be the good solution as we can fetch the assets from any path, and the theme can be installed in any supported folder.
Comment #11
pranav.aeer CreditAttribution: pranav.aeer commentedComment #12
idebr CreditAttribution: idebr at ezCompany commentedThe approach in #4 does not allow the Claro theme to be installed in all valid directories, eg. ./themes/contrib/claro
The approach in #10 does not work when Drupal is installed in a subdirectory.
Attached patch implements the suggestion in #6: copy all referenced files from Drupal Core to the Claro theme, so all references relative to the theme itself.
Note: the patch file contains binaries for png files so it must be applied using git:
git apply claro-path_to_images-3045216-12.patch
Comment #13
pranav.aeer CreditAttribution: pranav.aeer commentedThis may be the right solution, but what if those icons are updated in Drupal core update, we need to add those manually again under images/icons in Claro theme, right?
Comment #14
idebr CreditAttribution: idebr at ezCompany commented#13 Yes, the icons in Claro will have to be updated. However, the icons in Claro are likely to change compared to Drupal Core anyway: #3023308: Iconography style update
Comment #15
lauriiiComment #16
lauriiiComment #17
Lendude#12 makes sense to me, it is the only way to be sure that this works across all install dirs and as pointed out in #14 with #3023308: Iconography style update on the way, this was just a matter of time anyway.
Tested this locally and works good.
Comment #18
bnjmnmInformation regarding the files copied from core should be in a readme, and it can reference this issue.
Comment #19
lauriiiTo address the concern that we might accidentally forget about these icons and end up with some duplicates, I renamed the icons folder to core to signify that these icons are copied from core. I also added a readme file to explain that these icons were intended to be removed prior to inclusion in core.
Comment #20
bnjmnmReviewed the code and tested with Claro in different directories. Renaming the icons folder to core is a great idea and supplements the readme nicely.
I had to reroll the patch to include binary, but there's no other changes so I'll set this to RTBC once I confirm the patch applied properly.
Comment #21
bnjmnmBinary-rerolled patch applied properly, so setting this to RTBC
Comment #22
lauriiiThe theme already has a few existing core icons in the images/icons folder that should be moved to the core folder as well.
Comment #23
bnjmnmAddresses #22
Comment #24
lauriiiLooks good, thank you 👍
Comment #25
huzookaI agree
Comment #27
huzooka