Use inline SVG for drawer Drupal logo

Created on 15 April 2024, 2 months ago
Updated 1 May 2024, about 2 months ago

Instead of using an img tag for the static Drupal logo in the navigation, use an inline SVG. This saves a network request, increasing page speed, and provides greater styling flexibility.

Instead of:

<img alt="Navigation logo" src="/modules/custom/navigation/assets/images/logo.svg" loading="eager" width="40" height="40">

Include the logo_path in templates/navigation.html.twig directly.

The inline SVG markup should have the same accessibility as the img tag, so the img role should be added to the SVG markup, and an aria-label of 'Navigation logo'|t should be added.

logo.svg.twig has:

  • label
  • bg_color|default('#347efe')
  • fg_color|default('#fff')

reminder: 'label' maybe removed πŸ› navigation Logo does not communicated where the link is taking the user Needs work

πŸ“Œ Task
Status

Fixed

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States bronzehedwick

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @bronzehedwick
  • πŸ‡¨πŸ‡¦Canada SKAUGHT

    Converts logo.svg from file-system into 'data:image/svg+xml' in navigation_theme. also means we are not using extension.list.module service in template hook.

    other standing work with custom logo will apply still this way!

  • Pipeline finished with Success
    2 months ago
    Total: 215s
    #147248
  • Status changed to Needs review 2 months ago
  • Status changed to RTBC 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick

    I think this is a reasonable approach, removing the extra network request. We don't get the added styling benefits, but those don't have a plan to be taken advantage of right now, and this will remove refactoring. Marking RBTC!

  • Status changed to Needs work 2 months ago
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    Thanks for working on this! But I'm not sure about this: moving markup to a .module file makes things strange: as a themer, I'd expect this in a template itself with the rest of the elements if it's not an image. It works? Yes. Is this the expected place for this? Nope :)
    I'd go just add the SVG on the navigation.html.twig template and add any required logic there if the logo is replaced by a custom image or hidden.

  • Assigned to bronzehedwick
  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick
  • Issue was unassigned.
  • Status changed to Needs review 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick

    I created a new MR with a new approach. Is this more inline with what you're expecting @ckrina?

  • Pipeline finished with Success
    2 months ago
    Total: 291s
    #147407
  • πŸ‡¨πŸ‡¦Canada SKAUGHT

    IMO: this does give 'a themer' alot of ability to do things. however, i think we've confusing 'the graphic designer' from 'a themer'.

    -the ability to 'change color in twig' is advanced. if we have a color selection in the back end -- this would be usefull. but i might think its 'too much'
    -this is completely removing the IMG tag. this will break πŸ“Œ Create Image Styles for Nav Logo Active .

    ------
    @bronzehedwick. somewhere between. (:

     */
    function navigation_theme($existing, $type, $theme, $path) {
     ....
    
      $items['navigation'] = [
        'variables' => [
          'hide_logo' => FALSE,
          'logo_path' => $logo_path,
          'logo_width' => 40,
          'logo_height' => 40,
          'menu_content' => [],
          'menu_footer' => [],
        ],
      ];
    

    -. make path empty.
    - if twig sees empty load 'new icon twig' into PATH, otherwise PATH would be a custom logo (passed by NaviagaionRenderer).

  • Status changed to Needs work 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick

    Ah that makes sense, thanks @SKAUGHT. I pushed a commit that adds back that condition.

  • Pipeline finished with Success
    2 months ago
    Total: 230s
    #148265
  • Status changed to Needs review 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick
  • Status changed to RTBC 2 months ago
  • πŸ‡¨πŸ‡¦Canada SKAUGHT

    thanks!
    looks good. I'll move to RTBC!

  • Status changed to Needs work 2 months ago
  • πŸ‡¨πŸ‡¦Canada SKAUGHT

    @bronzehedwick
    - moving width/height to var seems not needed [as this doesn't recenter, re-size the actual paths]
    - also noticed viewbox is "0 0 32 32" -- i think should be 40?

  • πŸ‡¨πŸ‡¦Canada SKAUGHT

    SKAUGHT β†’ changed the visibility of the branch 3441090-use-inline-svg to hidden.

  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick

    moving width/height to var seems not needed [as this doesn't recenter, re-size the actual paths]

    This will re-size/re-center the entire graphic, as intended.

    All width/height sizing and any drawing code inside an SVG is relative to the viewBox. So the width and height on <rect fill="#347efe" width="32" height="32" rx="8"/> being set to 32 is the same as saying 100% of the width and height, since the viewBox is 32/32.

    Increasing the width/height on the <svg> element scales everything inside it based on the ratio of the viewBox, aka the S in SVG ;)

    also noticed viewbox is "0 0 32 32" -- i think should be 40

    As per above, the SVG is displayed at 40/40, but it's scaling baseline is 32/32 (aka, the viewBox). Changing the viewBox from 32/32 will make the internal scaling math off, in this case, adding extra white space around the SVG, optically shrinking it.

    You can test all this by changing the width and height values on the <svg> element.

  • Status changed to Needs review 2 months ago
  • πŸ‡¨πŸ‡¦Canada SKAUGHT

    width="{{ width|default(40) }}" height="{{ height|default(40) }}"
    we just don't need to be allowing the size to be changed. some one could send in a smaller size than the viewbox.

    re:viewbox. -- i've just got thrown off in my own head (viewport of svg)

  • Status changed to Needs work 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick

    Got it @SKAUGHT, I pushed a commit that hard codes 40 as the width and height.

  • Status changed to Needs review 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick
  • Pipeline finished with Success
    2 months ago
    Total: 256s
    #148502
  • Status changed to RTBC 2 months ago
  • πŸ‡¨πŸ‡¦Canada SKAUGHT

    nice.

  • Status changed to Needs work 2 months ago
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    This has a conflict with previous changes merged. Needs to be updated

  • Status changed to RTBC 2 months ago
  • πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

    Thanks for the merge conflict resolution @bronzehedwick. Looks good to me.

  • πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON
  • Status changed to Fixed 2 months ago
  • πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

    Merged to 1.x πŸŽ‰. Thanks!

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

Production build 0.69.0 2024