-
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
Use the EditorSkeleton component to layout the widgets screen #20431
Use the EditorSkeleton component to layout the widgets screen #20431
Conversation
Size Change: -442 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
f57e821
to
06bbecf
Compare
df8e9a5
to
235dc43
Compare
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.
Working fine in my testing! Thanks for this @youknowriad (and sorry for not doing it myself -- I had a local branch lying around, but styling was pretty broken 😅 )
Yes, the styling was not easy to get right because of those updates that were made to the sidebar of the Skeleton styles. |
Looks like there's some valid e2e tests breakage. |
</SlotFillProvider> | ||
<> | ||
<BlockEditorKeyboardShortcuts.Register /> | ||
<SlotFillProvider> |
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.
Is there any valid reason why all the wrapping components aren't included in EditorSkeleton
?
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.
No particular reason. Initially the component was more about the layout of the page, that said, this is an interesting idea. Makes me wonder if it's EditorSkeleton or just another component.
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.
Good call, it might be a better fit as a wrapper component. It all depends really :)
sidebar={ <Sidebar /> } | ||
content={ | ||
<> | ||
<Notices /> |
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.
Shouldn't EditorSkeleton
be also more opinionated about the placement of the Notices
component?
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 believe the answer is the same #20431 (comment)
@@ -75,26 +74,6 @@ html.block-editor-editor-skeleton__html-container { | |||
} | |||
|
|||
.block-editor-editor-skeleton__sidebar { |
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 support the idea of removing the CSS responsible for the different behavior on smaller screens that make it look like an overlay 👍
It looks like one of the failing tests has something to do with the removed CSS code for smaller screens: https://travis-ci.com/WordPress/gutenberg/jobs/290859611#L876
|
The only way to ensure this component is generic enough is to actually use it in multiple screens.
This PR does it for the edit-widgets, we need to follow-up on the edit-site as well.