Add a UI option to change or hide the logo on the Toolbar

Pushed commit. I'm not backender, so this is just a working POC.

Regarding this ticket - agreed with having setting to hide logo entirely. Regarding change logo or not - we already have this functionality. We can change logo of the admin theme -> and logo in navigation sidebar will be changed accordingly. Need to think if we should keep this behavior or re-create it from scratch exclusively for navigation module.

One more thing i don't like at all - is that i can upload currently 5000x5000 size image in `logotype` field in admin theme Claro - but on the front side we have:
1. no image style applied for logotype or responsive image group
2. Width / height attributes are static currently.

I think we have to have new image style 32x32 Scale effect. Probably even responsive image group. These problems can be resolved in this task

the image used is an SVG. the size and quality of the image are best (for sure) this way. lots of scope creep with image sizing here.

what this module currently does:

 */
function navigation_theme($existing, $type, $theme, $path) {
  $module_path = \Drupal::service('extension.list.module')->getPath('navigation');
  $icon_path = '/' . $module_path . '/assets/images/logo.svg';

  // Get the theme icon of the active administration theme.
  $admin_theme = \Drupal::config('system.theme')->get('admin');
  $logo_default = theme_get_setting('logo.use_default', $admin_theme);
  if (!$logo_default) {
    $icon_path = theme_get_setting('logo.path', $admin_theme);
  }

-> This loads its default (from the itself /assets/images/logo.svg) IF THE ADMIN THEME DOESN't HAve one set.

This is not a clear way to see how this icon is picked up from here.

Updated the MR to address @plopsec's comment.

Is there anything to address from #5? I'm trying to determine what the next steps are from there.

Nice! This looks like this right now:

Next step is the ability to replace the logo itself with another image, in the same form. We're providing an SVG, but they should be able to upload PNG, JPEG and SVG. I know allowing SVG can be a whole new conversation because of security reasons, so it could be moved to a follow-up so we prioritize the fact that they can customize it already.

add select options and js States.

todo: validate, save file, use file options in front end. The hiding is disconnected with this WIP as well.
note: new variables in navigation.settings.yml. i'm currently putting the file max size in the config, but no ui for this... *needs thought.

Let's use `Enable XY` as default instead of `Hide XY` and enable the option by default. It's way friendlier this way and less confusing.

cross-posted from Slack so we don't lose it

Thanks @SKAUGHT! Good idea having them as part of the same field to choose from, and using states.

Since there are only 3 options it'd be better to use radio buttons rather that a select on a usability perspective, because you can see the other options without having to interact with it. Plus it would allow for a description, that will be useful to explain the differences between them.

Also this way @saschaeggi's suggestion can be included and the option selected by default could be "Use Drupal logo" (instead of "Use default Navigation logo", because we aren't sure if the module name will end up actually being Administration or Admin Toolbar and it's suuuper long).

• validate has image
• detail max size
• detail recommend size and formats
• shows user the image size they attached*
• re-connects hide feature (from last commit)
• file usage in setttings submit
• *custom image size discovery in NavigationRenderer, and to theme function.

This does remove the option to use admin theme as an option. within slack conversion also including the fact that if some one sets the admin logo -- that size ratio of file is not suited for navigation. this list of problems keeps growing.

We give the User the outlines of what to do, let them make their choice. I do not want to deepen Navigation to require Imagecache/image.. We can use the imageFactory and simply get the image size.

Local QA:
I have tested locally with images above the 32x32 size. (ie: 1000x700).

• the image does work generally speaking (as we are outputting the img tag with w/h attributes). let the user take responsibility for their choice of customization, given this is direct file loading and imagefactory to grab size.
• even image 'just over recommended size' works comfortably. general responsive css kicks in. already has rounded corner applied via css.

todo's
- cleanup service injection in
- tests..
- if any suggest title/label string changes come up

Probably needs some file handling/error code handling edge-cases. Maybe more extensive validate on the file ajax when is SVG to pre-determine if has size..

As Drupal does not currently have an example of I've written in the parsing of the SVG to NavigationSvgTrait. I do not want to (be responsible to just) selecting a vendor package into core via this project.

Todo:
- needs actual tests (still)

Wow @SKAUGHT thanks for all the work here, it looks great!

Just to be sure my comment in #17 doesn't go unnoticed since in your todo lists don't mention it: this should use radio buttons instead of a select list for UX reasons.

@ckrina happy to do so!

I'll move to needs review for general UX of form and concerns with the way the logo works.

The current failed test from the Top nav settings test i'm not sure offhand why it failing.. but as Tests are still needed, would be cleanup then.

Thanks again @SKAUGHT for keepping this up to date!

Summary of the UX feedback by @rkoller and @benjifisher:

• Remove "Current image size (...)".
• Use "Recommended image dimension: 32 x 32 px" instead of "Image expected to be 32 x 32 px."
• Change radio button labels to something more concise:
  • Default logo
  • Custom logo
  • No logo
• Its is confusing that there is no image resize, specially with the current wording "current image size"
• The fieldset label and the label for the radios seem redundant. Can we keep "Logo options" (or "Icon options") and get rid of "Choose ..."?

Based on this, next steps would be:

• Keep "Logo options" and remove "Choose...".
• I'd move the "use custom logo" option at the bottom so when the image field appears is closer to it.
• Change radio button labels to something more concise:
  • Default logo
  • Custom logo
  • No logo
• Use "Recommended image dimension: 32 x 32 px" instead of "Image expected to be 32 x 32 px."
• Remove the line "Current image size (...)".

I'm also following the conversation opened in #security-discussion to see if we need to do any adjustments to allowing SVGs.

Apart of that I wonder if we should try to limit the image dimensions, resize raster images or give the option to provide a URL like themes do (t can be discussed&done in a follow-up so we don't block this).

(Giving credit for the Slack conversations in the #ux channel)

-i'd move the "use custom logo" option at the bottom so when the image field appears is closer to it.
^ a good thought for sure.

- "Logo options" and remove "Choose...".
overall: yes. we can certainly set the 'choose label for the radios to invisible as the element allows for. this will keep the fieldset legend as the overall group focus label.

- image max.
can do for true (jpg/png) files. WHERE SHOULD WE USE AS MAX? (:

SVG NOTE: validating the svg can have fails if the file is malformed. as this works now to get the size it works off the idea that if the svg
a.if has w/h attrib grabs them
b. if no w/h attrib we check for viewbox as a fallback
and as we fallback to 32, this work well.
I WORRY THAT: adding this in to the validation of the field/form may cause my issues where uses will get a 'can not ready SVG file' warning'.

- i'm also following the conversation opened in #security-discussion to see if we need to do any adjustments to allowing SVGs.
thanks, i understand. currently, we are taking a managed file and reading (simplexml_load_file) in the NavigationSvgTrait i've added here. otherwise we are just outputting directly to <img src="{{ logo_path }}"> in this usecase.

image resize:
we can not resize an svg in core/imagecache
As for jpg/png - at this time we have no dependency to image or imagecache at this point.

[opps]

sorry for the trail (:

We also have to create new image style 32x32 with Scale effect (no upscale). Scale effect because we can't crop logo, and no upscale because we have static box 32x32 pixels. So logotype in this box with such image style effect will be always centered vertically and horizontally, which is ok.

But current text "Recommended image dimension 32 x 32 pixels" is not so meaningful. Because actually we don't care. Proportions of logotype can be any. We have to say to the user that logotype will be resized proportially without cropping and it will be displayed in 32x32 box and centered. Something like this, but shorter ;)

Another feedback is about uploaded file. Instead of filename "logo.svg" we'd better have preview?
And btw just got cool idea - is that for preview we can use the same box 32x32 pixel for the user so before saving the form he can already check how logotype will look in such small box.

A quick point: Drupal does not do any of this (image preview) with a Theme Logo setting (and avoids 'Image/imagecache' dependency)
BTW: we're NOT locking anyone into a square image ratio either. again, user can decide if the logo works for them!

"Recommended image dimension 32 x 32 pixels" is not so meaningful. Because actually we don't care."
^i agree. and like theme logo, we let the user attach the file they already know what it is and let them use it. The User sees how well the image works (or doesn't..) and they can do as they please this way. We give a guide for size, it's the best we can do.

Edit: I think the MR needs to be updated with the lastest changes from 1.x

@SKAUGHT thanks for updating the issue summary with an screenshot of the last UI. It makes it way more easier to review it!

Thanks @kostyashupenko for you suggestions I agree with the image style, but I'd move it into a follow-up. Implementing the image style is not trivial (it'd have to check the file format first), and at the point the module is now is better to have a MVP implementation that is good enough and improve things later on.

@SKAUGHT I'm not totally following you with the SVG. But on a size perspective, we could handle whatever size the SVG (or the raster image) has with CSS with a wrapper. Not sure if this helps.

- image max.
can do for true (jpg/png) files. WHERE SHOULD WE USE AS MAX? (:

I'd say having the CSS wrapper can be enough? We just scale whatever is inside to a max width and height of 32px. And in a follow-up we can implement image styles for raster images.

@ckrina
i've just pushed the code spec fixes. thanks.

I'm mid-review here. Saw how the test was failing and went ahead and fixed it. It's green for me now locally 🤞.

Will continue with the review.

Great work so far!

See MR comments. Lot of little things. Also, lets move teh SVG handling for this to a follow up, as described. @KeyboardCowboy filed it: #3436520: Decide if and how we should support SVGs on the Nav Icon.

@m4olivei
thanks for the review points. I've pushed the removal of the SVG handling -- I'm short on some work time (pre-easter weekend) right now. maybe able to touch up other points tomorrow

i've seen the points on slack they've suggest for the SVG library/package. that is a good re-start point for SVG handling, for sure!

@starshaped
in case it's not clear quickly, i have only needed to add in the one sheet (with one style to cleanup the radio spacing) directly to this form only in order to not collide with other aspects.

This this would also be 'the place' to add in anything that could be added to the form in future, as certainly there could be more (fields/ideas) added to this form come then..

- It is just a css (not a pcss) but that could easily be refactored at a time it's needed to something compiled.

pulled in 1.x dev branch work

other branch work is certainly moving fast!

other branch work is certainly moving fast!

right!?

I've gone through the previous feedback and marked threads that have been addressed as resolved. Thanks for all of those! I've got a bit of time right now, I'm going to push up some commits for some of the remaining open items.

thanks @m4olive. I missed what you pointing to for 'file_upload_help'

Resolved all the threads. This is good to go for me!

thanks @m4olive. I missed what you pointing to for 'file_upload_help'

All good! It's hard sifting through big reviews in the Gitlab UI.

And merged! Thanks all for your work here, it'll be great to launch with this feature during the next weeks! 🎉

Let's keep the conversations for the SVG and image styles in the follow-ups:

• #3436520: Decide if and how we should support SVGs on the Nav Icon
• #3436526: Adjust custom navigation logo dimensions on upload

Sorry for being late to the party!

I would encourage to provide tests for this new feature. They'll probably be required at some point.

@plopesc good catch! I've created #3437821: [PP1] Add tests for the new feature to change the logo but I haven't added it to #3414937: [META] Write Automated tests for Navigation because no idea how to classify it. I'll let the experts handle it :P

Opened #3462829: Store the file path instead of ID for the navigation logo to try to make this a bit more flexible.