search

matejlatin / Gutenberg

A meaningful web typography starter kit.
Other
2.83k stars 159 forks source link

imgFixHeight / fixImgHeight #51

Open zackphilipps opened 7 years ago

zackphilipps commented 7 years ago

👋 I have included your project in my WordPress framework, Scratch. I love it!!!

There's just one issue I've found... the imgFixHeight function simply behaves too unpredictably. There are a lot of places where one would like to use an <img> tag and not have its height highjacked, such as a logo, an image carousel, etc... Obviously one could edit the function to suit one's needs, and I tried... However, there were still some instances of the page loading and certain images being set to height: 0. I'm wondering if it's worth it to make this function be as robust as it should be to justify it being there, or just discard it altogether. For Scratch, I've done the latter for now.

Again, thanks for your great work, and this is mostly an FYI!

matejlatin commented 7 years ago

Hey @zackphilipps thanks I think this could be something to look into for the next major update. Not sure when though... :(

ghost commented 7 years ago

I think the root of the issue is more about design theory than the actual JS function.

So I think the question should be: Is necessary to make everything follow a vertical grid on the web?

In print is a common practice but is also a static medium, and the web is an incredibly dynamic place, so is almost impossible to get that right. Also, let's consider that this is a generic library, so is impossible to know the exact use cases for everybody.

So I think the right way to handle vertical rhythm on the web is to not worry too much about element heights, only about elements spacing. In this case, we get to the simplest scenario where we don't have to deal with setting the right height for images (potentially getting distortion), for example, but this can also be applied to other tricky elements like dynamic heading sizes.

By getting consistent spacing on the elements we already get a good visual result, aligning everything to a grid is not important, nobody would notice it unless the lines of a grid are present.

I hope this all makes sense. What I'm saying is just a constructive critic, I also had to manage this stuff with Concise CSS and this is the route I took. I also created a lh unit for this same reason: https://github.com/KolceThompsonCo/postcss-lh and with it I just write how many "grid rows" of spacing I want for a certain element, without worrying about anything else.

What do you think @matejlatin?

matejlatin commented 7 years ago

Hey @jameskolce yes that was my original thought as well and I think I even wrote somewhere in the instructions that people shouldn't obsess too much with the height of the images. There should be a simple way to either enable or disable this JS fix but for the presentation purposes I left it in... might remove it in the future and only keep it in the examples.

ghost commented 7 years ago

I can check a way to enable/disable it easily this afternoon, will let you know if I get something!

ghost commented 7 years ago

So I just checked and I think the way to do it with minimal changes is just to add a global variable like var autofixImgHeight = false, and check for its value before calling the fixImgHeight function. Though splitting everything into a function an pass a configuration object would be a better solution.