Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚸 🩹 [SVG_Support: Logo Image] #680

Closed
wants to merge 1 commit into from

Conversation

wolfspyre
Copy link
Contributor

Add logic brace around img dimension math, so as to support SVGs.

Add warning discouraging specifying the same asset as both logo and logo_dark.

…o support SVGs. Add warning discouraging specifying the same asset as both logo and logo_dark.
@netlify
Copy link

netlify bot commented Oct 24, 2023

Deploy Preview for hugo-congo ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c1d4f0c
🔍 Latest deploy log https://app.netlify.com/sites/hugo-congo/deploys/653838724da88a00086709c5
😎 Deploy Preview https://deploy-preview-680--hugo-congo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jpanther
Copy link
Owner

I think this is a good change, I'm just wondering what hard coding the width and height to 25 for SVGs achieves? Doesn't this constrain the image to a tiny area?

@jpanther jpanther added the enhancement New feature or request label Oct 26, 2023
@wolfspyre
Copy link
Contributor Author

I thought the goal was to have the logo entity reside within the height of the menu element, which is ~26px so constraining to 25x25 to avoid being surprised

@wolfspyre
Copy link
Contributor Author

image

@jpanther
Copy link
Owner

The logo supports any image dimensions as some users prefer to hide the site name and use only a logo, in which case they provide large assets and the rest of the menu is laid out accordingly. It would be odd constraining the SVG but allowing any size for other assets.

@wolfspyre
Copy link
Contributor Author

ah! `k.. (the examples I saw when perusing the discussions all seemed to use it as a menu element. mah bad)

I could see the options of:

  • adding optional w/h keys to the header param element, to allow the user to specify the size if they wanted
[header]
  layout = "basic" # valid options: basic, hamburger, hybrid, custom
  # logo = "img/logo.jpg"
  # logoDark = "img/dark-logo.jpg"
  # logoWidth =
  # logoHeight =  
  • removing the specification of dimensions entirely for svg, delegating sizing to the viewbox element of the svg

got a better idea?

@wolfspyre
Copy link
Contributor Author

wolfspyre commented Nov 2, 2023

I played around with the svg logo a bit today while working on a dropdown menu implementation.

I'm open to feedback WRT how you'd like this improved; or what concerns you have.

Currently Hugo's image Width and Height methods1 are not able to assess the dimensions of an SVG.
Resultantly, the current {{ div $logo.Width 2 }} and {{ div $logo.Height 2 }} operations will need some massaging to work with SVGs.

as it sits, I see a couple options... if you see more, by all means, lets hear 'em.

  • hardcode x/y
    This is, admittedly, not the most robust solution. Just the simplest.

  • user-controlled dimension knobs:
    provide param keys for logoWidth and logoHeight; and use them to populate the img width and height properties in the logo.2 and just follow the current logic of resizing raster images proportionally3

Going much further than this will quickly tread into scope creep / yak shaving territory, as robustifying and do-what-I-mean-nerfing the user-facing knobs has a plethora of nuance4:

My gut says the most helpful thing to do is to simply emit a warning if AND5:

  • Both X and Y params have a user-provided value
  • the image is not a vector.

With something like:

You provided values for both Params.Header.logoWidth and Params.Header.logoHeight and the logo asset is NOT a vector.
If the generated image is distorted or misshapen, we advise setting only one of the two dimensions in your configuration

...And then just continuing to do what the user asked us to do.

Combing this behavior with clear documentation/instructions on usage. AUGHT be aligned with most users' expectations, I THINK?

This does kinda set the stage to start using the new image pipeline stuff, which has a bunch of new fun features.... but that's all out of scope for this simple change....

thots?

Footnotes

  1. at least on 0.119

  2. Default would be to leave them at 0, -1 or auto or something....

  3. May want to also automagically adjust the max-h-[Y] / max-w-[X] classes associated with the logo img... but maybe not

  4. If you only provide one dimension axis to Hugo's resizing tools, it's smart about retaining proportions...
    if you provide both X and Y, it will dutifully scale the image as you requested; even if that would contort the image... which is LIKELY not what most users would expect or desire.

  5. If its simple enough, perhaps only emit this warning if using the x/y provided would result in a change to the aspect ratio of the image. ¯\_(ツ)_/¯

@wolfspyre
Copy link
Contributor Author

wolfspyre commented Nov 2, 2023

I came up with a different, mechanism you might like a lil more?

layouts/partials/logo.html:

{{- if .Site.Params.header.logo }}
{{-   $logo := resources.Get .Site.Params.header.logo.source }}
{{-   if $logo }}
{{-     $w := -1 }}{{ if ne $logo.MediaType.SubType "svg"}}{{ $w = (div $logo.Width 2) }}{{ end }}
{{-     $h := -1 }}{{ if ne $logo.MediaType.SubType "svg"}}{{ $h = (div $logo.Height 2) }}{{ end }}
{{-     $logoWidth  := .Site.Params.header.logo.width | default  $w  }}
{{-     $logoHeight := .Site.Params.header.logo.height| default  $h }}
{{-     $logoDark := resources.Get .Site.Params.header.logoDark.source }}
    <a href="{{ "" | relLangURL }}" class="mr-2">
      <img
        src="{{ $logo.RelPermalink }}"
        width="{{ $logoWidth }}"
        height="{{ $logoHeight }}"
        class="max-h-[10rem] max-w-[10rem] object-left {{ if $logoDark }} flex dark:hidden {{ end }}"
        alt="{{ .Site.Title }}"
      />
{{-     if $logoDark }}
{{-       $dw := -1 }}{{ if ne $logoDark.MediaType.SubType "svg" }}{{ $dw = (div $logoDark.Width 2)  }}{{ end }}
{{-       $dh := -1 }}{{ if ne $logoDark.MediaType.SubType "svg" }}{{ $dh = (div $logoDark.Height 2) }}{{ end }}
{{-       $logoDarkWidth  := .Site.Params.header.logoDark.width | default $dw }}
{{-       $logoDarkHeight := .Site.Params.header.logoDark.height| default  $dh }}
          <img
        src="{{ $logoDark.RelPermalink }}"
        width="{{ $logoDarkWidth  }}"
        height="{{ $logoDarkHeight }}"
        class="max-h-[10rem] max-w-[10rem] object-scale-down object-left hidden dark:flex"
        alt="{{ .Site.Title }}"
      />
{{-     end}}
    </a>
{{-   end }}
{{- end }}

config/_default/params.yaml:

header:
  layout: 'grace' # hybrid, hamburger, basic.
  showTitle: false
  logo: 
    source: '/svg/Atticus-in-square-frame.svg'
    width: 100
    height: 100
  logoDark: 
    source: '/Wolfspyre_Logo.svg'
    width: 100
    height: 100
  showAppearanceSwitcher: true

lightsvg
darksvg

you like this more? I can update the PR; but I'd want to tweak a bit more, as currently this breaks existing behavior, since it converts the struct from a string to a dict.. I'd need to wire in support for what exists today (string as $logo/$logoDark) so it's still backwards compatible, and update doc to say that this is how one would support svg's... but turning the image construct from just a bare string to an object allows us to bolt in other stuff later (ie, output format/ image type to emit, pipeline operations to perform, caption, description, src_url for remote assets, etc ) if we want to... ¯\_(ツ)_/¯

@wolfspyre
Copy link
Contributor Author

for the record I'm not even sure I like this more, just trying to come up with other ways to solve the problem....

I was INITIALLY crazily hoping that if I added .Width/.Height as params of the image, that I could sidestep the .Width/.Height methods not working on vectors problem... but that was a stroke of insanity, not genius... however since I was already halfway thru banging it together, I figured I'd at least put it together enough to see if you like this approach more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants