Images with height attribute set do not preserve aspect ratio

[duanwilliam]

Not sure if this should have gone here or in the Docusaurus repo, but anyway.

while images are given max-width: 100%, their height is unspecified, so any height attribute assigned to an image stretches the image. this issue is seen e.g. on the main Docusaurus site at https://docusaurus.io/docs/advanced/architecture.

A simple fix would be to add height: auto to image tags as well.

[Josh-Cena]

This is regression after facebook/docusaurus#6989

@slorber I'm not even sure why you removed the height: auto—it's almost required because we use hardcoded width and height.

[slorber]

I don't understand the issue, can you show a screenshot where it's clearly visible?

I didn't remove height: auto everywhere, just images that are not in markdown

if an image is on the landing page, it's the user's responsibility to apply the appropriate CSS to it?

Not against reverting this change (maybe it's a good "CSS reset" that we can apply globally but I checked a few popular resets and this rule is not one of them) but I'd like to understand a concrete issue first

[Josh-Cena]

Weirdly the issue is gone on the production website now... It was surely an issue a week ago...

[slorber]

Afaik, max-width: 100% is declared globally by infima: https://github.com/facebookincubator/infima/blob/main/packages/core/styles/content/image.pcss

If both rules should go together, that height: auto rule should rather be added to Infima too?

BTW we also have users complaining about those global CSS rules being applied by Infima, so I'd rather actually remove those 🤷‍♂️

[Josh-Cena]

Hence this issue is in Infima🤓

those global CSS rules being applied by Infima

I think it's fine. We don't need to worry about every complaint, there will always be weird use-cases and we just need to make best choices for most people (i.e. easily-applied styles)

[duanwilliam]

Weirdly the issue is gone on the production website now... It was surely an issue a week ago...

I took a look and it seems the last commit with that bug was #7075 on Docusaurus (you can see it at https://deploy-preview-7075--docusaurus-2.netlify.app/docs/advanced/architecture), but the commit
after doesn't seem to change anything relating to the last deploy preview. perhaps there's an issue with the website deploy pipeline?

Afaik, max-width: 100% is declared globally by infima: https://github.com/facebookincubator/infima/blob/main/packages/core/styles/content/image.pcss

If both rules should go together, that height: auto rule should rather be added to Infima too?

I guess it's fine for the height: auto to be only within Markdown in Docusaurus because it's only really necessary due to the docusaurus mdx loader injecting width/height attributes. On the other hand, given that Infima is a framework for content-driven websites, I don't think it's unreasonable to have it in Infima as well, as having that rule can help precisely with issues like what was observed.