Olivero: Remove svg from block--system-powered-by-block.html.twig file

Why should we do that? Also as we're including the svg via an @include I'm not sure of the difference.

So that we can use this icon on other places as well. If we have this icon in Images directory, we can use this icon on different places by just including the icon in twig or adding as background-image.

By the current scenario we need to duplicate the icon on different places. Hope this answers.

I'm not fully sure about moving this to an /images or /assets folder.

If we do that, then we can't import it via @import in twig because it will be outside of the templates folder (so not accessible without the use of something like components module).

If we add it via <img src="path/to/images/drupal-logo.svg" /> then you can't do anything with it in CSS such as change path fill colours, etc.

Where it currently is may not be the best solution, but given we are working with only Drupal core and no contrib modules, it might be okay for now.

If we do that, then we can't import it via @import in twig because it will be outside of the templates folder

We can use include to add the SVG image. We're already doing it in different places in the Olivero theme. {% include "@olivero/../images/site-under-maintenance-icon.svg" %}

Check Olivero maintenance page template here

Whaaaaat? I never realised you could use .. to go back up a level and get out of the templates directory for include in Twig. That's opening up a who new set of possibilities. Sorry, that's an aside!

@Gauravmahlawat

This patch looks good to me, and the reasoning for using it looks sound. However, the patch is not currently applying for me. Can you check if it it needs a small re-roll please?

╰─ git apply -v ../_patches/3226047-3.patch
Checking patch core/themes/olivero/css/components/powered-by-block.css...
error: while searching for:
.block-system-powered-by-block svg {
    width: 0.875rem; /* 14 */
    height: 1.1875rem; /* 19 */
    vertical-align: top
  }

.block-system-powered-by-block svg path {
      fill: currentColor;
    }

.site-footer .block-system-powered-by-block a {
    color: #fff;
  }

error: patch failed: core/themes/olivero/css/components/powered-by-block.css:41
error: core/themes/olivero/css/components/powered-by-block.css: patch does not apply

I have re-rolled the patch for 10.1.x, Please review.

Thanks @Gauravmahlawat

This looks good to me, and a nice pattern for others to follow. Marking RTBC.

+++ b/core/themes/olivero/images/drupal.svg
@@ -0,0 +1,3 @@
+<svg width="14" height="19" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 42.15 55.08" fill="none" aria-label="{{ 'Drupal Logo'|t }}" role="img">

We can't use Twig syntax in the aria-label in an SVG file.

This means that by making this change, the accessible logo becomes untranslatable, I think?

Removed TWIG syntax. Please review

This means we lose translations of the accessibility label. @markconroy can you think of a way around this? We could name the file .svg.twig, but that reduces reusability. Is there another way?

This means we lose translations of the accessibility label. @markconroy can you think of a way around this? We could name the file .svg.twig, but that reduces reusability. Is there another way?

No other SVG image file has aria-label in the Olivero theme. Parent element of this SVG image has aria-label. <span class="drupal-logo" aria-label="Logo Drupal">

I think #22 looks good enough now. As @Gauravmahlawat says, no other SVG in using an aria-label in it in this theme, and we have a wrapper with an aria-label to announce what the SVG is.

I'd propose moving this back to RTBC. @longwave do you have any disagreement?

OK, as the parent has aria-label I think that should be sufficient for accessibility (and in fact perhaps better, as we don't repeat the label on the span and the svg itself).

Committed c1437df and pushed to 10.1.x. Thanks!

The reasoning and the considerations of aria labels are good to have on the issue. Nice work.

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