Adjust custom navigation logo dimensions on upload

- 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

todo:
- helpfull info

opening for review.

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

ckrina → closed merge request !205

ckrina → reopened merge request !205

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

Some suggestions, mostly code style.

updated help content

all notices addressed. thanks!

Sorry, missed that the render array cache should be invalidated by changes to the image style. Added comment to MR.

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?

@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.

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.

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!

Moving to Needs work after getting Navigation into core.

reminder 'too small'

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.

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.

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

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();
 ....

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.

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

Added a first POC of the new approach.

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

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

Test coverage for icon resizing on upload added.

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

• 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.

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.

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.

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.
  

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.

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 review . It's not a requirement for this issue. I've updated the issue summary. @SKAUGHT or @plopesc can correct me if I'm wrong.

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

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

Thank you for stepping up @m4olivei!

I'm not entirely sure about this.

Say there is a design change to increase the image size to 45*45 pixels, then all the existing uploaded images would be too small. Could we not use an image style instead? It would mean navigation module needs to ship a new image style, and then render the image via that, but should be less code overall too.

Say there is a design change to increase the image size to 45*45 pixels, then all the existing uploaded images would be too small. Could we not use an image style instead? It would mean navigation module needs to ship a new image style, and then render the image via that, but should be less code overall too.

The first pass at the implementation here did use an image style. However, that approach was abandoned for reasons. See #35 - #39. Namely @plopsec had the following concerns with the image style approach, which weren't sufficiently addressed, and thus we shifted to the current approach.

• 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

I'd argue that the design is pretty stable, and should there be a future adjustment to the image size, sites would still have the recourse of re-uploading their custom logo.

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

This at least could be handled with validation to prevent deletion when navigation module is installed.

re: Say there is a design change to increase the image size to 45*45 pixels,
✨ Center small navigation logo on collapsed state Needs review would be the fallback to address images that are too small.

hmm those are good answers. Still not sure about this, but moving back to RTBC and will try to get a second opinion from another committer (or someone else can just commit it too of course).

I went through the same though process as you @catch and got the same "umm I don't know but I don't have a clearly better solution" conclusion. I should have commented, it could have saved you a bit of time.

I'll leave the commit to someone else too, this one is a bit out of my scope.

There's one other issue here, I didn't review ✨ Add a UI option to change or hide the logo on the Toolbar Needs work before it went in and just realised it now thinking about this one.

Requiring a file upload here means that this has to be done on the production site, and then the file entity and file itself synced back to a development environment, and then the config exported.

The theme logo UI lets you upload a managed file, but the configuration stores the file path, and it also lets you enter a file path directly. This means even if you rely on an uploaded file, it's possible to change the local configuration to point to the file path without having to know the specific file ID.

This would then provide a way for site owners to use SVGs, which would partially solve 📌 Decide if and how we should support SVGs on the Nav Icon Active then.

The theme logo UI lets you upload a managed file, but the configuration stores the file path, and it also lets you enter a file path directly. This means even if you rely on an uploaded file, it's possible to change the local configuration to point to the file path without having to know the specific file ID.

🤔 Great point. It makes good sense that the navigation logo would follow the theme logo UI more closely. Although the theme logo doesn't do any management of the size of the image, neither is enforcing a size on upload, adjusting the size on upload, or applying an image style on output. Are you suggesting that we follow that same approach wholesale, in which case this issue is not necessary? Or are you still OK with the approach here and wanting to add the consideration for better config export support in a followup issue?

This would then provide a way for site owners to use SVGs, which would partially solve #3436520: Decide if and how we should support SVGs on the Nav Icon then.

Agree it would help that issue.

I see now that a follow up was created for 📌 Store the file path instead of ID for the navigation logo Needs work .

. Are you suggesting that we follow that same approach wholesale, in which case this issue is not necessary

No not really, just the config management part. 📌 Holistic logo image insertion and dimensions in themes Active is an existing issue with the theme logo that we shouldn't re-introduce to navigation.

@catch the existing implementation already uses file entities, so changing that here feels out of scope and a followup exists for it; the problem at hand is to restrict/adjust the size, which the PR looks to solve.

Updated credit

Just to confirm again, I'm fine going ahead here and continuing in 📌 Store the file path instead of ID for the navigation logo Needs work .

In the interest of moving forward, committing this one.

Committed 3b29a90 and pushed to 11.x. Thanks!
Committed f27c142 and pushed to 11.0.x. Thanks!
Committed 83500c6 and pushed to 10.4.x. Thanks!

nod_ → closed merge request !8247

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