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.