-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Pages: display content frame in mobile when canvas is not edit #60409
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +136 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
I think there's some CSS we need to do to leave some space for the "hub" on the top. This is probably something we already do in other places too. |
Fixed that, also for Templates&Parts pages: Gravacao.do.ecra.2024-04-03.as.11.12.55.mov |
@include break-medium { | ||
margin-top: 0; | ||
} | ||
} |
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 wonder if there's a way to solve this in a generic way (like in the "mobile" area of layout). Do you think it works for all the use-cases or are there exceptions?
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.
Pushed something that works across all existing screens.
Gravacao.do.ecra.2024-04-05.as.10.56.04.mov
Flaky tests detected in 9215564402478648bcedb646dbc9f93dd54022cc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8536188641
|
9215564
to
e03b2d1
Compare
<div | ||
className="edit-site-layout__mobile" | ||
style={ { | ||
maxWidth: widths?.content, |
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.
If widths become necessary for mobile, I think the router should declare them for this specific "area" the same it does for "content" — e.g.: maxWidth: widths?.mobile
.
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.
So this change has no impact?
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.
Nope. This only affected to pages that whose mobile area had content + had a width set for the content area (the existing logic only accounted for the list layout):
Gravacao.do.ecra.2024-04-05.as.11.13.47.mov
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.
Was testing a different thing in trunk
and this is what happened with this rule in place:
Gravacao.do.ecra.2024-04-05.as.11.21.34.mov
@@ -43,12 +43,6 @@ | |||
} | |||
|
|||
.edit-site-page-patterns-dataviews { | |||
margin-top: 60px; |
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.
Instead of adding this bit of CSS to every page that displays the content frame in a mobile viewport, this PR moves it to the hub component.
@@ -26,6 +26,13 @@ | |||
} | |||
} | |||
|
|||
.edit-site-layout__header-container:has(+ .edit-site-layout__content > .edit-site-layout__mobile) { |
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.
Depending on what component is displayed, the header sibling would be different:
- a page component in the content frame:
.edit-site-layout__content > .edit-site-layout__mobile > .edit-site-page
- the sidebar frame:
.edit-site-layout__content > interface-navigable-region edit-site-layout__sidebar-region
- the editor:
.edit-site-layout__content > .edit-site-layout__mobile > .edit-site-editor__interface-skeleton
@@ -52,7 +52,9 @@ export default function useLayoutAreas() { | |||
mobile: | |||
canvas === 'edit' ? ( | |||
<Editor isLoading={ isSiteEditorLoading } /> | |||
) : undefined, | |||
) : ( |
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.
Display the list on mobile.
Ah, right, forgot the hub was part of the editor as well. I'll take a look at that. |
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.
LGTM 👍
Yeah, it is — they overlap in |
… fix overlapping hub in Templates (WordPress#60409) This extracts some existing styles in the Patterns page to the Hub component, so any Page component (Pages, Templates, Patterns) has the same styles. Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Part of #55083
Related #59659
What?
When navigating to the Pages page in mobile, display the content frame instead of the sidebar. This also fixes the templates screen that was obscuring the hub styles.
Gravacao.do.ecra.2024-04-05.as.10.56.04.mov
Why?
Otherwise, the user is unable to navigate through the pages.
How?
Testing Instructions
Go to Pages and using a mobile viewport (for example, use the browser's devtools).