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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ccasals created an issue. See original summary.

lauriii’s picture

Title: Composer based build causes missing asset(s) » Asset paths pointing to core/misc folder assume Claro is installed in core/themes
Issue summary: View changes
ccasals’s picture

Ah, 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 .

ccasals’s picture

These are the places I could find pointing to "misc". These updated paths should work if claro is in the core/themes directory.

ccasals’s picture

Status: Active » Needs review

Since the intended final destination for claro is core/themes, this is ready for review!

lauriii’s picture

Any 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.

pranav.aeer’s picture

As the final path for claro will be core/themes, I've just applied patch provided on #4 and tested, it works for me.

pranav.aeer’s picture

Status: Needs review » Reviewed & tested by the community
lauriii’s picture

Status: Reviewed & tested by the community » Needs work

I think it would be great if we could come up with a solution that would (temporarily) allow installing the theme in any supported folder.

pranav.aeer’s picture

I 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.

pranav.aeer’s picture

Status: Needs work » Needs review
idebr’s picture

The 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

pranav.aeer’s picture

This 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?

idebr’s picture

#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

lauriii’s picture

Issue tags: +alpha blocker
lauriii’s picture

Priority: Normal » Critical
Lendude’s picture

Status: Needs review » Reviewed & tested by the community

#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.

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

Information regarding the files copied from core should be in a readme, and it can reference this issue.

lauriii’s picture

Status: Needs work » Needs review
FileSize
17.99 KB
20.32 KB

To 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.

bnjmnm’s picture

Reviewed 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.

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

Binary-rerolled patch applied properly, so setting this to RTBC

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

The theme already has a few existing core icons in the images/icons folder that should be moved to the core folder as well.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
921 bytes
20.9 KB

Addresses #22

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thank you 👍

huzooka’s picture

I agree

  • huzooka committed c533e60 on 8.x-1.x authored by bnjmnm
    Issue #3045216 by bnjmnm, lauriii, pranav73, ccasals, idebr: Asset paths...
huzooka’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.