Skip to content

Limit height of image to page height#217

Merged
joanise merged 3 commits intomainfrom
dev.image-height
Jul 5, 2023
Merged

Limit height of image to page height#217
joanise merged 3 commits intomainfrom
dev.image-height

Conversation

@dhdaines
Copy link
Copy Markdown
Collaborator

@dhdaines dhdaines commented Jul 4, 2023

This seems to effectively fix the issues with displaying images on iPads, and also enforces what I think is a reasonable height on uploaded images, namely, 55vh, which is in theory also the same maximum height as a page (it seems to make them a bit smaller because nobody can comprehend where the size of an element actually comes from in CSS)

@dhdaines dhdaines requested review from joanise and roedoejet July 4, 2023 20:04
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 4, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://ReadAlongs.github.io/Web-Component/pr-preview/pr-217/
on branch gh-pages at 2023-07-05 13:55 UTC

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

Nice, thanks for tackling this!

On Studio-Web, things are better, though not perfect yet.
On the downloaded Web Component, though, it's not fixed. I wonder if your patch is failing to get triggered in that case?

Studio-Web screen shots:

This one is great, with the whole tall image visible:
image

This one is even taller, 1100 pixels tall, and still gets cropped a bit at the bottom (you should completely see my two hand-drawn corners at the bottom):
image

Web-Component screen shots:

On the downloaded Web-C, the original problem persists:
image
image

@extend %themed-bg;
img {
height: 100%;
max-height: $page-size + 0vh;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate CSS! A huge amount of work, for a one-line change in the end!

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

Same results on the iPad: in Studio-Web, I can now see my tall image. But in the downloaded html, viewed in Edge on the iPad, it's still blank.

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

To make sure that wasn't the problem, I checked, and assets/bundle.js did get updated in the PR preview, so that's not it.

@dhdaines
Copy link
Copy Markdown
Collaborator Author

dhdaines commented Jul 5, 2023

To make sure that wasn't the problem, I checked, and assets/bundle.js did get updated in the PR preview, so that's not it.

Hm. The only way to know for sure is to inspect the image in the browser to see if the new CSS is getting picked up. I was having a lot of problems getting nx to actually rebuild things, some bogus caching was getting in the way.

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

I'm surprised you were having trouble. I now just run this command:

npx nx run-many --targets=serve-test-data,serve-web-api,serve,serve-fr,serve-es --projects=web-component,studio-web --parallel 6

and everything starts reliably for me.

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

Right, yeah, in Studio-Web:
image

In downloaded Web-C:
image

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

OK, I can confirm it was because the bundle did not get updated: it contains the old code:

... .page__col__image img{height:100%;max-width:95%;object-fit:contain}...

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

And it's because we need to run

npx nx bundle web-component

and commit the results!

Doh!

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

Although the workflow that creates the PR Preview does run that command, and so do our publish and release workflows, so I don't quite understand why we have to do it by hand.

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

Argh! I updated the bundle. It's now fixed locally. But not on the PR preview. Help! @roedoejet

@joanise
Copy link
Copy Markdown
Member

joanise commented Jul 5, 2023

Got it! It's because we're fetching the bundle from the live version, not the PR preview:
image

It's only a bug in the PR preview, then, which is fetching assets from /assets/... rather than /pr-previous/pr-217/assets/...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants