Adjust custom navigation logo dimensions on upload

Created on 27 March 2024, 4 months ago
Updated 8 July 2024, 11 days ago

Problem/Motivation

Navigation allows to upload a custom logo to be included at the top of the bar.
Nowadays, the only restriction applied is related to image weight, but not for dimensions. That means that image bigger thatn te expected placement are being adjusted via CSS.

For a better resource management, would be great to adjust the custom logo dimensions to the logo placeholder to avoid loading unnecessarily big images.

Proposed resolution

The proposal here is to use a similar approach as we already have when uploading a picture to an image field that exceeds the maximum allowed size for the field.

When image is bigger than 40x40px, it would be automatically scaled during upload and a message like this would be shown to the end user:
The image was resized to fit within the navigation logo expected dimensions of 40x40 pixels. The new dimensions of the resized image are 40x40 pixels.

Following this path, code would be much simpler, reducing the number of possible points of failure and we would be safer assuming that original image can be used directly as navigation logo.

To give more flexibility, the icon size could be set as properties in the navigation.settings config entity, but not shown. So contrib modules or advanced site administrators could modify them.

Remaining tasks

None.

User interface changes

Adds message during file upload to notify user the image will be automatically resized if above expected size.

API changes

Adds image width/height as Configuration (no UI additions for these). For flexibility around image sizes changes tied to config rather than code, same as current file-size restriction already.

Data model changes

Uses Configuration to establish expected size of navigation logo.

📌 Task
Status

RTBC

Version

11.0 🔥

Component
Navigation 

Last updated about 9 hours ago

No maintainer
Created by

🇺🇸United States KeyboardCowboy

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

Merge Requests

Comments & Activities

  • Issue created by @KeyboardCowboy
  • First commit to issue fork.
  • Merge request !205Added config yml file with image style → (Open) created by kostyashupenko
  • Pipeline finished with Failed
    4 months ago
    Total: 179s
    #131180
  • Status changed to Needs work 3 months ago
  • 🇨🇦Canada SKAUGHT

    - lets make this config an optional install
    - lets work in a softer-dependency to imagecache being applied if service available.
    - lets add some help information about this

  • Pipeline finished with Failed
    3 months ago
    Total: 186s
    #138978
  • Pipeline finished with Success
    3 months ago
    Total: 210s
    #138977
  • 🇨🇦Canada SKAUGHT

    todo:
    - helpfull info

  • Pipeline finished with Success
    3 months ago
    Total: 223s
    #139059
  • Pipeline finished with Success
    3 months ago
    Total: 304s
    #139063
  • Pipeline finished with Canceled
    3 months ago
    Total: 44s
    #140856
  • Pipeline finished with Success
    3 months ago
    Total: 209s
    #140858
  • Pipeline finished with Success
    3 months ago
    Total: 260s
    #141116
  • Pipeline finished with Success
    3 months ago
    Total: 277s
    #141131
  • Status changed to Needs review 3 months ago
  • 🇨🇦Canada SKAUGHT

    opening for review.

    maybe easier for people to look at current file:
    https://git.drupalcode.org/issue/navigation-3436526/-/blob/3436526-creat...

  • 🇮🇳India ehsann_95

    The image style is getting applied after following the Help topic. The Help topic is to the point.

  • Pipeline finished with Success
    3 months ago
    Total: 261s
    #141828
  • Pipeline finished with Success
    3 months ago
    Total: 219s
    #141844
  • Pipeline finished with Success
    3 months ago
    Total: 226s
    #141865
  • Status changed to Needs work 3 months ago
  • Some suggestions, mostly code style.

  • Pipeline finished with Canceled
    3 months ago
    Total: 15s
    #141901
  • Pipeline finished with Canceled
    3 months ago
    Total: 3s
    #141902
  • Pipeline finished with Canceled
    3 months ago
    Total: 15s
    #141903
  • Pipeline finished with Canceled
    3 months ago
    Total: 7s
    #141904
  • Pipeline finished with Failed
    3 months ago
    Total: 194s
    #141906
  • Pipeline finished with Canceled
    3 months ago
    #141929
  • Pipeline finished with Success
    3 months ago
    Total: 209s
    #141932
  • Pipeline finished with Canceled
    3 months ago
    Total: 152s
    #141960
  • Pipeline finished with Success
    3 months ago
    Total: 220s
    #141962
  • Pipeline finished with Canceled
    3 months ago
    Total: 175s
    #142955
  • Pipeline finished with Failed
    3 months ago
    Total: 208s
    #142962
  • Pipeline finished with Canceled
    3 months ago
    #142976
  • Pipeline finished with Canceled
    3 months ago
    Total: 63s
    #142980
  • Pipeline finished with Failed
    3 months ago
    Total: 193s
    #142981
  • Pipeline finished with Success
    3 months ago
    Total: 357s
    #142998
  • Status changed to Needs review 3 months ago
  • 🇨🇦Canada SKAUGHT

    updated help content

  • Pipeline finished with Success
    3 months ago
    Total: 220s
    #143071
  • 🇨🇦Canada SKAUGHT

    all notices addressed. thanks!

  • Pipeline finished with Success
    3 months ago
    Total: 330s
    #143077
  • Pipeline finished with Success
    3 months ago
    Total: 223s
    #143118
  • Pipeline finished with Success
    3 months ago
    Total: 222s
    #143123
  • Pipeline finished with Success
    3 months ago
    Total: 228s
    #143242
  • Status changed to Needs work 3 months ago
  • Sorry, missed that the render array cache should be invalidated by changes to the image style. Added comment to MR.

  • Status changed to Needs review 3 months ago
  • Pipeline finished with Success
    3 months ago
    Total: 246s
    #148130
  • Pipeline finished with Failed
    3 months ago
    Total: 249s
    #148132
  • Pipeline finished with Success
    3 months ago
    Total: 255s
    #148129
  • Pipeline finished with Success
    3 months ago
    Total: 266s
    #148131
  • Pipeline finished with Failed
    3 months ago
    Total: 262s
    #148140
  • Status changed to Needs work 3 months ago
  • Pipeline finished with Failed
    3 months ago
    Total: 233s
    #148274
  • Pipeline finished with Canceled
    3 months ago
    Total: 33s
    #148276
  • Pipeline finished with Canceled
    3 months ago
    Total: 99s
    #148278
  • Pipeline finished with Success
    3 months ago
    Total: 290s
    #148281
  • Pipeline finished with Success
    3 months ago
    Total: 221s
    #148352
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • One comment on MR for the wrong variable being used as cacheable dependency.

    Las thing: there are no automated tests added for this new functionality. Are tests required?

  • Status changed to Needs review 3 months ago
  • 🇨🇦Canada SKAUGHT

    @godotislate
    thank-you. i clearly needed more coffee yesterday (:

    Re tests:
    As there are serveral tickets in motion around the logo itself 📌 Add tests for the new feature to change the logo Active was opened as a bucket once options are finalized.

  • Pipeline finished with Success
    3 months ago
    Total: 220s
    #149054
  • Pipeline finished with Success
    3 months ago
    #149360
  • Pipeline finished with Success
    3 months ago
    Total: 277s
    #149361
  • Status changed to RTBC 3 months ago
  • LGTM.

    One note, though, that is apparently unrelated. When I test latest Navigation 1.x-dev (#11b42e3) against latest Drupal core 10.3.x-dev (#e03ae01) with standard profile, the third level menu items become unclickable. They do seem to work fine against core 10.2.5. I haven't checked against core 11.x because currently drush does not have a compatible version.

    Uploaded screen recording: https://www.drupal.org/files/issues/2024-04-17/Screen%20Recording%202024...

    I looked quickly through the issue queue and didn't see anything, so if someone else can confirm or reproduce, an new issue can be opened for that.

  • 🇨🇦Canada SKAUGHT

    i've been working in 10.2 myself. there is a new tag (today) and some followup issues around keyboard-a11y ticket work are open.
    I did see that same thing for a moment, then re-flushed my drupal caches and browser cache --then was okay..

    (: i'm sure that would be called out more directly from dev line in the next few hours..

    I'm confident it's not coming from this branch of work. cheers!

  • 🇪🇸Spain ckrina Barcelona
  • Status changed to Needs work 3 months ago
  • 🇪🇸Spain ckrina Barcelona

    Moving to Needs work after getting Navigation into core.

  • 🇪🇸Spain ckrina Barcelona
  • 🇨🇦Canada SKAUGHT

    reminder 'too small'

  • 🇪🇸Spain ckrina Barcelona
  • Pipeline finished with Success
    2 months ago
    Total: 141s
    #174397
  • 🇪🇸Spain plopesc Valladolid

    Been thinking about this issue and I think that using Image Styles here adds an extra layer of complexity that might not be necessary.

    Having an image style we need to assume these risks:

    • Image style could be modified by site administrators, leading to unexpected results
    • Image Style could be deleted by site administrators, so we cannot assume that it is present and need to have a fallback in every place we load the custom logo image
    • Would force to add anew dependency on drupal:image for Navigation

    My proposal here is to use a similar approach as we already have when uploading a picture to an image field that exceeds the maximum allowed size for the field.

    When image is bigger than 40x40px, it would be automatically scaled during upload and a message like this would be shown to the end user:
    The image was resized to fit within the navigation logo expected dimensions of 40x40 pixels. The new dimensions of the resized image are 40x40 pixels.

    Following this path, code would be much simpler, reducing the number of possible points of failure and we would be safer assuming that original image can be used directly as navigation logo.

    To give more flexibility, the icon size could be set as properties in the navigation.settings config entity, but not shown. So contrib modules or advanced site administrators could modify them.

  • 🇨🇦Canada SKAUGHT

    plopesc
    the precursor problem is Image (core module) is enable and Servers have php's GD Library Enabled. As i've thought around this: we should just enforce a 40x40image and not offer to resize. And 'IF' down the line we allow svg --> same only verify size, let the user use the correct file in the first place.

  • 🇪🇸Spain plopesc Valladolid

    Good points @SKAUGHT

    Let me try to clarify the approach to try to address your concerns.

    The automatic resize image approach does not require the image module to be enabled because the image manipulation API is in \Drupal\Core\Image class.
    If the Image manipulation API fails to transform the image due to the unlikely scenario where GD is missing or any other internal fail, we can show an error indicating that image has to be of the specified dimensions, like FileImageDimensionsConstraintValidator does.
    If SVG support is added in the future, we might need to create a specific path for that

  • 🇨🇦Canada SKAUGHT

    u mean something like:

    use Drupal\Core\Image\ImageFactory;
    ...
    $image_factory = \Drupal::service('image.factory');
    $image = $image_factory->get($file->getFileUri());
    
    if ($image->isValid()) {
       // Scale the image to fit within 40x40 pixels, maintaining aspect ratio.
       if ($image->scale(40, 40)) {
          // Save the scaled image.
          $image->save();
     ....
    
  • 🇬🇧United Kingdom catch

    Agreed that image style is tricky. SVGs definitely would need their own code path if added eventually, as I found out in 📌 Holistic logo image insertion and dimensions in themes Active - getting SVG dimensions is very different.

  • 🇨🇦Canada SKAUGHT

    yes, a correctly defined width and height attributes with fallback to svg viewBox if defined. i had mocks on that on other branches but we didn't have a sanitize routine worked out there.

    just a reminder also we do need the correct size for the img as the logo is eager loading. we do output the correct image size from existing routing with a custom image.

    (: lol. so much work for such a small picture

  • Merge request !8247Issue #3436526: Adjust custom logo dimensions → (Open) created by plopesc
  • 🇪🇸Spain plopesc Valladolid

    Added a first POC of the new approach.

    It will require some adjustments and tests, but I think it is working as expected.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 264s
    #187340
  • 🇨🇦Canada SKAUGHT

    SKAUGHT changed the visibility of the branch 3436526-create-image-styles to hidden.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 230s
    #189623
  • Pipeline finished with Success
    about 2 months ago
    #189632
  • Pipeline finished with Failed
    about 2 months ago
    #189639
  • Pipeline finished with Success
    about 2 months ago
    Total: 512s
    #189652
  • Status changed to Needs review about 2 months ago
  • 🇪🇸Spain plopesc Valladolid

    Test coverage for icon resizing on upload added.

    I think this is ready for a first round of reviews.

  • 🇮🇳India Prashant.c Dharamshala
    • Custom logo changes are not working for me.
    • Tried to add images of 200x200 as well
    • It is always resizing the images to 40x18 or so not to 40x40 or higher and logo image not getting replaced even after cache clear.

    Let us wait for reviews from others as well.

  • 🇪🇸Spain plopesc Valladolid

    This MR is trying to address logo issues in parallel with 🐛 Custom Navigation logo is disconnected from new Layout template Fixed .

    Problems with custom logo visibility are being worked on that issue.

    The aim of this issue is to resize images, if possible, to fit in the 40x40 expected space for logo.

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 560s
    #198679
  • Pipeline finished with Success
    about 1 month ago
    Total: 511s
    #198692
  • 🇪🇸Spain plopesc Valladolid

    Code updated after 🐛 Custom Navigation logo is disconnected from new Layout template Fixed was added to core. Minor conflicts had to be addressed.

    This is ready for a final round of reviews.

  • Status changed to RTBC about 1 month ago
  • 🇨🇦Canada SKAUGHT

    This is looking good across our general needs and has tests!

    • An oversized 16:9 image shows user notice of action.
    • undersized is left alone and the dimensions are correct for eager loading.
  • First commit to issue fork.
  • 🇳🇿New Zealand quietone New Zealand

    I read the issue summary, comment and the MR. There is a remaining task of another issue but it isn't clear if that needs to be completed first. Does it?

    There is discussion of the solution at #35->#37 and I didn't find any unanswered questions.

    I made 3 minor changes to the MR, none of which are in code. Commit checks pass locally so tests should be fine here. Therefor, I am leaving this at RTBC.

  • First commit to issue fork.
  • Pipeline finished with Success
    13 days ago
    Total: 714s
    #216999
  • 🇨🇦Canada m4olivei Grimsby, ON

    I read the issue summary, comment and the MR. There is a remaining task of another issue but it isn't clear if that needs to be completed first. Does it?

    I don't believe so. It doesn't need to be completed first. The added functionality here addresses logos that are too big. However, just like it is on 11.x at present, a user can still upload a logo smaller than the recommended dimensions, and we need a fix so we don't break the layout, which is the subject of Center small navigation logo on collapsed state Needs work . It's not a requirement for this issue. I've updated the issue summary. @SKAUGHT or @plopesc can correct me if I'm wrong.

  • 🇨🇦Canada SKAUGHT

    no, it required by, nor first. it would be a different set of tests at well.

  • 🇪🇸Spain plopesc Valladolid

    I don't see any of the mentioned issues as blockers.

    Thank you for stepping up @m4olivei!

Production build 0.69.0 2024