search

okkur / syna

Highly customizable open source theme for Hugo based static websites
https://syna.okkur.org/demo/
Apache License 2.0
250 stars 134 forks source link

hero: Image height is ignored #734

Closed c-mauderer closed 4 years ago

c-mauderer commented 4 years ago

Is this a BUG REPORT or FEATURE REQUEST?: bug

What happened: The height of the asset in the hero fragment is ignored. It's completely irrelevant what I use. I always get the same height. That's true for Firefox and Edge. I use Syna v0.16.2 so the fix from the similar issue #520 is already included.

What you expected to happen: The height should work.

How to reproduce it (as minimally and precisely as possible):

+++
fragment = "hero"
weight = 1000
background = "dark"
particles = false
minHeight = "500px"

title = "Some Title"
subtitle = "Subtitle"

[header]
  image = "oven-4757093.jpg"

[asset]
  image = "logo_light.svg"
  # width = "300px" # optional - will default to image width
  height = "100px" # optional - will default to image height
+++

Anything else we need to know?: From what I can tell it seems that the "height: auto" from the css overwrites the "height=..." from HTML. If I change the lines

{{- if .height }} height="{{ .height }}"{{- end -}}
{{- if .width }} width="{{ .width }}"{{- end -}}

to

{{- if or .width .height }}
style="width:{{- if .width }}{{.width}}{{- else -}}auto{{- end -}};height:{{- if .height }}{{.height}}{{- else -}}auto{{- end -}}"
{{- end -}}

in layouts/partials/fragments/hero.html everything works like expected. But I'm far to few web developer to tell whether that's a correct solution.

Environment:

stp-ip commented 4 years ago

We'll take a look.

c-mauderer commented 4 years ago

No need to hurry. I noted that a height in % doesn't work like I expected (maybe depending on the browser). With any other units I can use the width attribute without problems to set the size.

But it's still a bug and therefore I thought that I should report it.

stp-ip commented 4 years ago

So 150px for both height and width work? So the main issue is percentages not working?

That definitely helps with pinning it down. Might be a browser specific thing, but we'll see if we can get this working.

c-mauderer commented 4 years ago

Seems my last comment was misleading:

I't is quite possible that a percent value for a hight doesn't work at all. I thought it's percent of the screen but it seems that it is percent of the block containing the image. So the height of the block would depend on the content. And the image is part of that content. That would be a circular dependency and most likely is undefined.

From what I have seen, it seems that the "height" or "with" attributes of an image might shouldn't be used with the current html standard but should be defined by css instead. See https://html.spec.whatwg.org/#presentational-markup and https://html.com/attributes/img-height/. The second page seems to recommend to not really use a height besides "auto".

So maybe instead of fixing the height it should be removed? But I really have to few web experience to tell that for sure what is the right way.

stp-ip commented 4 years ago

Thanks for the context.

Usually we use css, but with user defined config it's much harder in Hugo to get the configuration into CSS so defining it inline within the markup works best. Similar to how sometimes inline styling is the unfortunate way to go, even if overall it's a bad idea.

Will let @mpourismaiel jump in on the above fix and a final decision on feasability for percentage. My personal view is that percentage might be harder to support overall as that might depend on the parent block, which relies on the overall page dimensions different on each device. So specific values might work best as far as my own experience goes.

mpourismaiel commented 4 years ago

I changed the sizing from html attribute to an style attribute. I personally would have preferred the change to happen in css but it's a really complex thing to achieve. Also it would be nice to change the object fit to cover instead of the initial value. @stp-ip You can check it out by changing the height and setting object-fit: cover; on the image img tag. It wouldn't be breaking since height wasn't working previously and object-fit requires both values to work properly. Let me know what you think and if you agree, create an issue and I'll get on it.