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

Fixed / optimized some things

Hi @Gauravmahlawat, @kostyashupenko nice catch and work, I have checked the file, now we can see the raw svg is removed from block--system-powered-by-block.html.twig.
Moving to RTBC

The issue summary says:

The block--system-powered-by-block.html.twig file have raw svg file in code. We should move this file to images folder then include this file.

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

@Gauravmahlawat I have checked the patch after patch svg has been removed
Thanks for the patch

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

As mentioned in #8 what is the "why" if you can update the issue summary with that please.

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.

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!

Also, I've updated the issue summary with the reasoning behind this change request.

@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 think it's the spaces just before the closing brackets that might be the issue.

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, if it is to be used as a raw SVG?

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?

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.