-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Expand main content area to viewport when zoomed out #59512
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasmussen @MaggieCabrera Sorry to ping you, but maybe with your CSS wizardry skills you can think of ways to make this work for all themes. Otherwise I think we'll have to rely on themes to add it. Or maybe this is fine for block-based themes if they don't use a custom stylesheet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this css conditional to having no patterns inserted instead? We only care about this when the page is empty, otherwise if we insert a pattern that doesn't fill the whole viewport height we are introducing fake space that is not really there in the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm catching up after a bit. What does this property do?
I'd agree we want to ideally avoid a theme needing to add anything at all in order for the zoom prop to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it break themes? Doesn't this CSS be just in editor? Also what makes it work in zoom out mode only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some misunderstandings:
Or maybe we can rely on block-based themes never styling
is-root-container
, in which case this CSS may be safe to use as it won't clash with anything. It still relies onheader
,main
andfooter
being top-level blocks though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasmussen when zooming out the content is scaled. We would want that the scaling of an empty page to not cause a small zoomed out patch containing header a small nothing and a footer but instead when zooming out to enlarge vertically the shape to obtain header a large nothing footer. The purpose of this trickery is to make it obvious that the big nothing is where the assembly of patterns will happen (assuming an empty page is zoomed out for building it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid enough, I think there's some nuance to explore there in terms of what that means for the blue pattern insertion plus buttons as well, and where they show up. I.e. to your point, we certainly don't want those to stack right next to each other. But we also want to avoid an accordion behavior where the document is always at least full-viewport-height when empty, and then it collapses as soon as you add a single section pattern. So I'd be tempted to apply a min-height instead of flex magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasmussen I think this does exactly what you say: #59512 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of this PR is to not have that. Maybe @richtabor can explain this better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, this simply ensures the footer is at the bottom of an empty page, or rather when there is not enough content to push it there—and only when zoom-out mode is engaged. It helps align the interface with expectations of where content (i.e. patterns) will be inserted.
This is not a "fit" view, but rather an initial state when there's not a full page of content.