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 size we have:
1. no image style applied for logotype
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.

ckrina → closed merge request !181

ckrina → reopened merge request !181

ckrina → closed merge request !181

ckrina → reopened merge request !181

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
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 differences between.

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)
• *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

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

I have tested locally with images about 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' work comfortably.

I would say this is worth some general review.

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.

ckrina → credited benjifisher → .

ckrina → credited rkoller → .

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.

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 pixels for the user so before saving the form he can already check how logotype will look in such small box.

@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 → closed merge request !181

ckrina → reopened merge request !181

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

I separated the SVG conversation here: 📌 Decide if and how we should support SVGs on the Nav Icon Active

Move image styles work to its own ticket: 📌 Create Image Styles for Nav Logo Active

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: 📌 Decide if and how we should support SVGs on the Nav Icon Active .

@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) directly to this form only in order to not collide with other aspects.

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.

m4olivei → closed merge request !181

m4olivei → reopened merge request !181

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:

• 📌 Decide if and how we should support SVGs on the Nav Icon Active
• 📌 Create Image Styles for Nav Logo Active

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 📌 Add tests for the new feature to change the logo Active but I haven't added it to 📌 [META] Write Automated tests for Navigation Active because no idea how to classify it. I'll let the experts handle it :P

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