-
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
Global Styles: Add block preview component in global styles #45719
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
Thanks, this is going to help a lot! |
0c72e2a
to
37a6f84
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.
This is looking great! It's testing well for me.
I've got a few comments:
-
I'm not sure how obvious it is that this is a preview of the block. I think maybe a border around the block preview container would help make the preview boundary more obvious. (Maybe like the image on the right here?)
-
There seems to be a lot of padding on the iframe, especially at the top, e.g. the Quote block looks like it has a lot of space above its preview. I think making the preview area more obvious (e.g. with a border) may help this.
-
Some blocks don't preview anything, such as: List Item, Post Comments Link, Post Comments Count, Post Author Name, Post Title. I guess this is because there is no example content for them? (Although I don't think they all need to work for this to be merged, I was just highlighting it)
-
The Navigation preview shows the mobile menu, but I'm unable to click on it to open it:
- When there is no preview available, maybe we should show something like a disabled preview area, so it doesn't look like the preview is just broken/not showing? This could be a follow-up.
packages/edit-site/src/components/global-styles/block-preview-panel.js
Outdated
Show resolved
Hide resolved
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.
Thanks for working on this! I focused on reviewing the code since @mikachan already has left great feedback on the functionality.
packages/edit-site/src/components/global-styles/block-preview-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/block-preview-panel.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/block-preview-panel.js
Outdated
Show resolved
Hide resolved
37a6f84
to
c633153
Compare
@mikachan @noisysocks Thanks for the review.
Introduced the boundary to make it obvious.
This is due to double spacing ( container padding and block vertical margin ). Addressed it using defaultPadding from example which is set to only a few blocks.
Right, because they doesn't have an example. They can be added in a separate PR.
The preview is not interactive. It doesn't show hover state for button for example.
mmm, makes sense and it will be consistent. It could be a follow-up |
c633153
to
d84626d
Compare
Can you add a |
I think we should also use some opacity or blend mode on the border so that it works ok in situations with custom background colors. |
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.
Does setting scale
and defaultPadding
have any effect for the inserter's block preview? or is it just for this new instance of the block preview?
Should we update the block API documentation to document scale
and defaultPadding
?
packages/edit-site/src/components/global-styles/block-preview-panel.js
Outdated
Show resolved
Hide resolved
d84626d
to
9353e51
Compare
No. They are optional. Inserter block preview doesn't define them and so doesn't have any effect. Only preview in styles panel use them.
Updated the docs with examples here |
I have done some testing with different colors with the current changes and following are the screenshots. When a dark background is set, the border is almost not visible. I might want to try the blend-mode for the border on both of these together in a separate PR. |
9353e51
to
3734478
Compare
8ddde35
to
b77a33d
Compare
Reverted the new APIs. This PRs has this tiny change in docs. Thanks for the wonderful feedback. Suggestions are addressed. Note: Took a change related to center alignment of the block out of this PR as it needs to be commonly addressed in all the preview components such as |
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.
Thanks for iterating on this @madhusudhand! This looks close!
I'm not sure if this was discussed before, but do we really want the extra margin and in general so much spacing in the previews? If we for example add background it looks like this:
I'll leave this for the designers 😄 --cc @jameskoster
Good point, the margin can make the display a bit awkward, especially if you have something like a Quote where the margin is so large that it pushes the text out of the preview viewport. Does the preview need to be full-size, or can we scale it like the previews in the Inserter? A side note: Does the preview window background inherit from global styles, or is it hard-coded to be white? |
The extra space is not entirely due to the margin. It is margin + padding set by custom CSS in specific themes, to add root level padding. Now that #35607 is fixed, individual themes needs a fix to remove the extra padding. With no such extra padding in themes such as Twenty-Twenty Three, the preview with background is similar to how it looks in the editor, and it is expected I guess. (theme: TT3)
It inherits from the global styles.
With scaling it is no longer |
b77a33d
to
1e0f8a5
Compare
update viewport widths for few blocks add border to preview block and convert viewport width to scale fix border radius and address minor review comments address review comments
1e0f8a5
to
5856410
Compare
Took this for a quick spin, looks good, thanks for pushing this one. Even if there are rough edges, it brings so many benefits to it so it would be good to land this even if we need to follow up with some iterations. Two quick observations. It might be good to set box-sizing: border-box; on the block when inside the preview, so it doesn't break out of boundaries when applying border: And secondly, margin really doesn't translate well in the small preview: It might be useful to zero out all margins in this preview. It isn't big enough to show the effect anyway. If need be we could explore a followup showing a larger preview on hover, something like that. |
I think that's a complex issue to solve holistically and could lead to unexpected results. For example even if we add the params/styles to zero out margins and/or padding, there could be blocks that theme styles add borders etc..(ex. |
Nothing to block this PR, as noted, it's such a cool feature and it would be nice to see it soon! |
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.
Thanks for iterating on this one @madhusudhand! Code wise looks good and IMO we can land and iterate to improve. If there is no objection from designers to land this as first version let's 🚢
* add block preview in global styles panel update viewport widths for few blocks add border to preview block and convert viewport width to scale fix border radius and address minor review comments address review comments * revert scale * revert defaultPadding and alignment APIs * remove dropcap for paragraph
Nice work @madhusudhand ! |
Note for my future self: double-checked on the Dev note label. Block api changes for |
What?
This change adds block preview in global styles.
How?
Note: there are few issues observed in the examples of block such as
image
which renders more spacing. However, it seem to be a different issue.Testing Instructions
Screenshots or screencast
Issues
Partial fix for #42919