Holistic logo image insertion and dimensions in themes

Created on 1 March 2023, about 2 years ago

Problem/Motivation

The logo in olivero is hard-coded. All logos in core are either hard-coded or we _only_ render the image path. Never dimensions. We should. And we might want a holistic solution that passes from theme config the logo and the logo dimensions. For good measure, we could even add loading="eager". That's the default, but adding it is more explicit and instructive.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

10.1

Component
Theme 

Last updated about 23 hours ago

Created by

heddn Nicaragua

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

Merge Requests

Comments & Activities

  • Issue created by @heddn
  • 🇬🇧United Kingdom catch

    I wonder if this was an unexpected regression from #908282: Remove unnecessary I/O from theme_image() (although that issue was good in general).

    The main issue with this is that we don't have anywhere obvious to store the dimensions, the theme settings just point to a file on disk, there's no file reference or similar.

    We could do what #908282: Remove unnecessary I/O from theme_image() used to do in preprocess and cache it somewhere maybe? It's a bit hacky, but it's minimal and would help page rendering quite a lot.

    Or we could add dimensions to the theme configuration form/config object and prepopulate them from the file info if not explicitly set? This would have the advantage of not needing an extra cache get but you could end up with weird issues if you change your theme logo file without updating the form.

  • 🇬🇧United Kingdom longwave UK

    Could we render the logo alone in its own template, preprocessing the height/width, and leverage the render cache?

  • 🇬🇧United Kingdom catch

    I couldn't remember where the logo is rendered, but it's in SystemBrandingBlock - we could do it directly in there I think, just add the attributes when building the render array. It might even be OK to rely on render caching for of the block, although would need to check that.

  • Status changed to Needs work about 1 year ago
  • 🇬🇧United Kingdom catch

    Made a start on this but it's quite painful.

    The pain is mostly because of system_preprocess_block and

    core/modules/system/templates/block--system-branding-block.html.twig
    

    In SystemBrandingBlock we do things 'properly' and use '#theme' => 'image', but then that preprocess and the template just extracts the URI from the image render array and renders the image with hard-coded HTML. Why?

    This means any added attributes get stripped.

    Promoting to a bug because there's no way at all to do this generically at the moment.

    I got around this in stark by adding an extra variable for the rendered image, then using that in the template, it is really ugly but I can't think of any other way except adding an entirely new block and deprecating the old one.

  • Pipeline finished with Failed
    about 1 year ago
    #132375
  • Pipeline finished with Failed
    about 1 year ago
    Total: 502s
    #132495
  • Status changed to Needs review about 1 year ago
  • 🇬🇧United Kingdom catch

    Deprecated the 'site_logo' variable, adding a change record and a release note, this ought to be reviewable now.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 527s
    #132510
  • Status changed to Needs work about 1 year ago
  • 🇬🇧United Kingdom catch

    That svg logic isn't happy, removed that and also switched to using the image factory for non-svg images.

  • 🇳🇿New Zealand quietone

    Trying to remove the 'See more' from the snippet

  • 🇬🇧United Kingdom longwave UK

    This won't make it into 10.3.

  • First commit to issue fork.
  • Pipeline finished with Failed
    15 days ago
    Total: 2217s
    #451623
  • Merge request !11538Inline logo SVG. → (Open) created by nmangold
  • 🇺🇸United States nmangold United States

    The current MR, https://git.drupalcode.org/project/drupal/-/merge_requests/7241, dumps "boo" to the page. Consequently, the height and width attributes are not added to the image. This is because SVG is not a supported type in GDToolkit. The image factory method $image->isValid() returns false. I assume that is why the URI is extracted and used in the hard-coded <img> tag instead of the render array.

    I created a new MR, https://git.drupalcode.org/project/drupal/-/merge_requests/11538, that proposes checking the extension and printing the SVG inline, which includes the height and width attributes.

  • Pipeline finished with Failed
    14 days ago
    Total: 581s
    #452314
  • 🇺🇸United States smustgrave

    So currently there are now 2 MRs, 1 should be hidden or closed

    Also issue summary appears to be complete.

    Will probably need test coverage

  • 🇺🇸United States nmangold United States

    Added a screenshot of the logo as an inline SVG.

  • 🇺🇸United States nmangold United States

    nmangold changed the visibility of the branch 3345259-logo-image-dimensions to hidden.

  • Pipeline finished with Failed
    7 days ago
    Total: 206s
    #458260
  • Pipeline finished with Failed
    7 days ago
    Total: 148s
    #458277
  • Pipeline finished with Failed
    7 days ago
    Total: 135s
    #458283
  • Pipeline finished with Failed
    7 days ago
    Total: 544s
    #458284
  • 🇺🇸United States nmangold United States

    Now that I better understand this, I added code similar to the previous, hidden branch. The default logo in the themes is now an SVG. However, a non-SVG image could be configured or uploaded using the theme settings. Some of the tests also use the old Druplicon logo.

    For SVGs, we cannot render the image using the image factory, because GDToolkit does not support SVGs. So, we need both solutions, one for images supported by GDTookit and one for images not supported (SVGs).

    I changed the release notes for deprecating the site_logo and described the additional site_logo_rendered for rendering the <img> tag with attributes.

    Also, I added tests covering both situations.

  • 🇬🇧United Kingdom catch

    One question on the MR.

  • 🇺🇸United States nmangold United States

    Added a screenshot of the logo as a PNG, which renders the <img> tag with attributes including dimensions.

  • 🇺🇸United States nmangold United States
  • 🇺🇸United States nmangold United States
  • Pipeline finished with Failed
    about 24 hours ago
    Total: 156s
    #462867
  • Pipeline finished with Failed
    about 24 hours ago
    Total: 155s
    #462868
  • Pipeline finished with Canceled
    about 24 hours ago
    Total: 281s
    #462871
  • 🇺🇸United States nmangold United States

    I removed the release notes. The new variable is no longer used. The extension logic was moved to the block plugin which allows the templates to simply print the site_logo variable.

  • Pipeline finished with Failed
    about 23 hours ago
    Total: 1787s
    #462878
  • Pipeline finished with Failed
    about 23 hours ago
    Total: 576s
    #462898
  • Pipeline finished with Failed
    about 23 hours ago
    Total: 602s
    #462902
  • Pipeline finished with Failed
    about 21 hours ago
    Total: 1018s
    #462928
Production build 0.71.5 2024