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

BlockPreview: Patterns with VH units scale oddly #50449

Open
richtabor opened this issue May 8, 2023 · 17 comments · May be fixed by #54285 or #51485
Open

BlockPreview: Patterns with VH units scale oddly #50449

richtabor opened this issue May 8, 2023 · 17 comments · May be fixed by #54285 or #51485
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@richtabor
Copy link
Member

I added a couple new patterns as core patterns, one of which uses the vh unit to set the height of a Cover block. This causes the pattern's BlockPreview to continuously expand in height, resulting in an oddly displayed pattern.

CleanShot.2023-05-08.at.17.05.40.mp4

Related in part to #43865. And the same issue occurs in the Site Editor's Zoomed Out mode, as indicated here: #49299

Thoughts on how we can resolve this?

@richtabor richtabor added the [Type] Bug An existing feature does not function as intended label May 8, 2023
@talldan
Copy link
Contributor

talldan commented May 22, 2023

Thoughts on how we can resolve this?

I guess there's a feedback loop going on here:

  • The height of the preview is based on the viewport's height, and in this case the viewport is the iframe
  • The iframe's height is based on the content of the preview

When one changes it causes the other to change, which in turn causes the other to change and so on.

It might be possible to limit how much the iframe updates its own height, but that could also cause some legit height changes to not happen. The height update could also be throttled, but that would just slow down the amount it expands 😄

Definitely an interesting one!

@kevin940726 kevin940726 self-assigned this Jun 13, 2023
@kevin940726
Copy link
Member

Seems related to #38181. I don't think this issue is solvable with iframe, since the iframe height is always 100vh, and we can't render anything visible outside the iframe. Any block that has a minimum height over 100vh will cause the preview to grow infinitely. This comment explains it very well. The only way I can think of for these kinds of patterns to work within iframes is somehow parsing the CSS in the preview and replacing any instance of vh with computed units. Seems like an over-complicated work TBH 😅 .

Another quick fix I can think of is to acknowledge that some patterns can't be shown fully in a preview and instead render a sort of "more below" indicator at the bottom. Something like...

Screenshot

This is obviously not the final design but just a mockup of an idea. I can put up a draft PR and let anyone interested play with the design to find a better balance.

@kevin940726 kevin940726 linked a pull request Jun 14, 2023 that will close this issue
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jun 14, 2023
@richtabor
Copy link
Member Author

Any block that has a minimum height over 100vh will cause the preview to grow infinitely.

Would 99vh work?

@kevin940726
Copy link
Member

Would 99vh work?

If it's only a single element with 99vh then it will. But if it's partnered with any non-viewport values like a 30px margin-bottom then it probably won't. 99vh + 30px will be the height of the iframe (100vh), which means 1vh is equal to 30px, and the height will be 3000px. There's currently a max height of the iframe to stop it from growing bigger than 2000px.

This is also the case for the pattern shown in the video of this original issue. IIRC, it has a height of 75vh and some paragraphs below. Adding both of them makes the height of the iframe exceed the 2000px limit, thus making it look weird 😅 .

@miminari
Copy link
Member

miminari commented Jun 30, 2023

I think this move is irritating to users. How about removing the transition CSStransition: all 0.3s ease 0s; temporarily until we can come up with a better solution?
The interaction is very nice, but I don't feel that it is inevitable for the function.

@ellatrix
Copy link
Member

In my opinion, we should set a max height to the preview, perhaps at the same ratio of the viewport height based on the width of the preview. There are previews that are taller, but then the rest of it will appear "under the fold" as it would appear on the front-end and in the editor? That seems reasonable to me.

@kevin940726
Copy link
Member

we should set a max height to the preview

We already set a max height to the preview, currently it's hardcoded to 2000px though 😅 .

perhaps at the same ratio of the viewport height based on the width of the preview

Could you elaborate what you mean here? Do you mean the ratio of the current users' browser viewport?

@ellatrix
Copy link
Member

Do you mean the ratio of the current users' browser viewport?

Yes :) I think we should make the ratio the same. The downside is that you cannot preview "under the fold", but that seems reasonable to me. It's just a preview.

@richtabor
Copy link
Member Author

The downside is that you cannot preview "under the fold", but that seems reasonable to me. It's just a preview.

I agree.

@kevin940726
Copy link
Member

The downside is that you cannot preview "under the fold", but that seems reasonable to me. It's just a preview.

Yep, and it's already the case. I don't think the issue is even solvable as per #50449 (comment). Any block that has height > 100vh will not be shown with auto-scaling iframe.

@kevin940726
Copy link
Member

I implemented #50449 (comment) (IIUC) in #54285 so that we can see it in action. Let's discuss there if that's the approach we want to take at this stage.

@richtabor
Copy link
Member Author

I misunderstood the "under the fold" bit. I wasn't thinking that it was connected to your direct viewport, but a viewport size of the preview itself. If we have a max height on those, and the rest is runoff, that's fine (basically like how patterns in the "Choose a pattern" modals work).

@tellthemachines
Copy link
Contributor

The same bug was reported on Trac here. From the report:

Occurs when any cover block's min. height is set to a vh value in the Site editor settings, and I select the Styles sidebar and from there "Browse styles" to view the theme's style presets - when the template or page "zooms out" to preview style changes, the cover block's height expands vertically perpetually making the preview hard or even impossible (if 100 vh is entered as min. height).

@talldan
Copy link
Contributor

talldan commented Dec 19, 2023

In #56806 we discovered this bug also happens in the experimental Zoomed Out view in the editor canvas.

@t-hamano
Copy link
Contributor

An issue with the zoomed out view was resolved by #59334. I'm wondering if a similar approach could be applied to block previews as well.

@ellatrix
Copy link
Member

The difference with the preview is that it doesn't have a fixed viewport height (expands based on content). For zoom out mode we keep the same viewport height and scale the html element inside the iframe (I removed the expanding). So the only fix would be to set a fixed max height to the previews?

@richtabor
Copy link
Member Author

richtabor commented Feb 28, 2024

So the only fix would be to set a fixed max height to the previews?

Do we do this for block previews anywhere already? I recall perhaps within the starter patterns modal, or templates view?

@kevin940726 kevin940726 removed their assignment Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: In progress
7 participants