-
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
Try/even better previews #16873
Try/even better previews #16873
Conversation
// Dynamically calculate the scale factor | ||
useLayoutEffect( () => { | ||
const { clientId } = blocks; | ||
timerId = setTimeout( () => { |
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.
Let's add a comment to explain why we need the setTimeout (the block list being rendered async..)
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.
It's pretty grim and I felt bad when I added this in my original PR. Is there no way to detect the rendered
event?
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 solution I proposed (see the PR comment) solve some of it but I think the block list is rendered twice. First at init with an empty block list, and then a second time with the full block list. that's because of how BlockEditorProvider
works. (init on mount). I think that's the main issue here. So when first rendered, the block list is empty.
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 no way of propagating the change of state within BlockEditorProvider
from "empty" to "I have some blocks" via a callback or something?
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.
Great work on exploring this route again. You made some improvements over my original version of this. However I still think there is a fundamental flaw that is outside of your (our) control namely that we cannot reliably target the DOM element which represents the visual width of the Block.
See #16113 (comment)
packages/block-editor/src/components/block-preview/test/index.js
Outdated
Show resolved
Hide resolved
If we remove the idea behind of querying the element to preview I think it won't work at least in the way it was initially thought. For instance, the While the element that we use to compute the scale is the next one: |
I don't see any alternative but to remove it. Sorry 😢 I fully understand the problem (I've worked on it all week 😄 ) but it isn't a problem we can solve in this way. We've hit that roadblock already on #1611. We cannot control or know in advance how the DOM of Blocks will be created. The best we can do is Riad's suggestion of |
I think for the button we can solve this by making that container "div" take the size of its content. (especially for the "unselected" state) |
But that's the problem, Raid. We make that assumption for the |
I think there's two things. The fact that the wrapper of the button doesn't take its container's size can be considered a bug IMO because that's not how the frontend looks and the editor should match the frontend for blocks. The other aspect here is that depending on the content of the block, the preview might not look great. (Say you type just a few words in a quote compared to a full line text). For this case, a solution (that is needed elsewhere as well) is to add a new API to blocks to be able to define "an example content" instead of the content shown in the block while opening the block switcher. (that should be a separate PR though) |
Actually, I just noticed that there's a full-width div for the button block in frontend too. you can argue that in that case, it's "normal" that the button block shows up as very small because its content is the whole div (what if it has a background image defined by a theme for instance, as silly as it is, it's part of the content). This is to say IMO, we should tweak the button block to make its container take the size of its content in both editor and frontend. |
Props @marekhrabe.
We'll revisit when we have a better idea what specifically we should be testing
The "Build artifacts" test keeps failing, not sure if that is something that's caused by this PR? |
@obenland That's one of two things |
…berg into try/even-better-previews
What a team effort 🎉 |
Thank you SO much for all your help! |
💋 |
HOOLY wow. 👏👏👏 |
👏 👏 👏 👏 |
* First stab at a proof of concept. * Attempt to programmatically determine dest dimensions * Add README stub. * Updates to use default args for width and height Addresses #16113 (comment) * Update class namespaces to match package Addresses #16113 (comment) * Add width and height as Effect Hook deps Addresses #16113 (comment) * Use correct `createRef` from @wordpress/element Was previously using React createRef. Addresses #16113 (comment) * Revert "Update class namespaces to match package" This reverts commit 24252fa887986548839e7d9a6fe2639807ed0bc6. * Migrate to useRef hook * Adds basic unit test Still a WIP * Refactor to avoid unecessary state and simplify implementation of scaling * Add some padding to the preview Avoids Blocks butting up against the edges of the preview, especially if the preview wrapper has rounded corners which can cause the Blocks to be clipped. * Refactor to utilise JS template strings * WIP: Adds zoom for small individual blocks With small individual Blocks showing the preview at “actual size” (even when scaled to fit) is a bit odd. Everything still looks very zoomed out and far away. Updates to check DOM for largest element within the Block and use that width to calculate a “zoomed” scale value so that the contents of the Block itself is shown zoomed in. Still WIP * Updates to target block contents more reliably and to calc entire box model in width * block-preview: render scaled preview in a shadow dom * try: setting styles to shadow dom elements * Revert "try: setting styles to shadow dom elements" This reverts commit ea573f9. The reason being that there is no benefit of using shadow DOM and it introduces additional complexity. * Removes attempt at auto zoom on Block contents After much experimenting we’ve decided that attempting to reliably detect the visual size of the Block’s visual contents is impossible. This is due to inconsistencies in the DOM markup of each Block. See #16113 (comment) * Updates to remove scale by default in favour of opt in via prop As discussed here #16113 (comment), there is little point in having thumbnail preview and large popover preview showing the same thing. Update to introduce a prop to opt in to scaling the preview. By default the preview is no auto-scaled. Apply `true` value to `BlockPreview` to ensure that Block Styles popover preview is scaled to fit. * Adds scaleFactor prop to provide control for non dynamically scaled preview * Adjust scaleFactor for Block Styles preview * Fix to ensure hooks are called unconditionally to obey rules of Hooks See https://reactjs.org/docs/hooks-rules.html * Removes padding and border from preview on designer advice * Only apply dimensions to preview content when dynamically scaling * block-preview: a different approach attempt. * Correct typo in util function name * Seek the firstChild of the Block DOM node * Tidy up effect timers and improve comments * Dynamically calculate widths and offsets of preview container Removes hardcoded values. * Updates to account for widths of all Blocks passed into to preview Preivously we only considered a single Block scenario. However, the recent addition of multi block support for Previews means we need to ensure we’re measuring the widths of _all_ Blocks. * Revert class additions to Blocks * Adds optional scope to DOM utils. Use to limit scope in Block width comparison. * Update packages/block-editor/src/components/block-preview/index.js Co-Authored-By: Riad Benguella <[email protected]> * Allow prop based opt out from scaling * Fix to ensure smallest Block is found even if smaller than container Preivously we were default to the container width being the largest item. In fact we always want to determine the largest Block contents element and use that to determine the scale. * Remove unecessary Math.min usage * core/button: adjust dims of button wrapper * apply vertical alignment * set scale factor to 0.9 * rollback testing purpose comment * cpre/button: keep adjusting styles for preview * rename vars and css class names * adjust rebase * restore isScaled factor scale * set the scale adjustment in the proper place * update README file * core/button: tidy edit preview styles * coer/button: tweak button preview * preview-block: ensure make the preview visible. * block-preview: hide dropzone * core/button: adjust preview for thumb and full sizes * core/button: set same width for preview * core/button: because it's !important * core/button: set nowrap button in preview * core/button: set nowrap onlu for button * core/quote: adjust quote size * Make previews overflow to the bottom. Currently still has a bug where the large `.block-editor-block-switcher__preview` pane behaves like it's previewing a taller block, when it's not * Revert "core/button: set nowrap onlu for button" It breaks previews of long buttons This reverts commit 758468c. * Fix unit test * core/button: apply nowrap only to buttons * Try a fixed canvas width * Fix blocks editor styles * core/button: centering preview * core/button: adjust only into teh styles preview * block-preview: remove scaleAdjustment property * block-preview: hide inserte element in preview * block-preview: just pick up the first block to scale * block-preview: fix set tall class. X position (wip) * popover: remove commented lines * Refactor the preview * Remove debug code * simplify preview resets * Fix the preview recomputing * Vertical alignining small blocks * block-editor: restore viewportWidth as a property * Update Readme docs Props @marekhrabe. * Remove tests for now We'll revisit when we have a better idea what specifically we should be testing * block-preview: simplify comparision * Remove readme updates that break tests * Update docs properly. See 6f29c27
* First stab at a proof of concept. * Attempt to programmatically determine dest dimensions * Add README stub. * Updates to use default args for width and height Addresses #16113 (comment) * Update class namespaces to match package Addresses #16113 (comment) * Add width and height as Effect Hook deps Addresses #16113 (comment) * Use correct `createRef` from @wordpress/element Was previously using React createRef. Addresses #16113 (comment) * Revert "Update class namespaces to match package" This reverts commit 24252fa887986548839e7d9a6fe2639807ed0bc6. * Migrate to useRef hook * Adds basic unit test Still a WIP * Refactor to avoid unecessary state and simplify implementation of scaling * Add some padding to the preview Avoids Blocks butting up against the edges of the preview, especially if the preview wrapper has rounded corners which can cause the Blocks to be clipped. * Refactor to utilise JS template strings * WIP: Adds zoom for small individual blocks With small individual Blocks showing the preview at “actual size” (even when scaled to fit) is a bit odd. Everything still looks very zoomed out and far away. Updates to check DOM for largest element within the Block and use that width to calculate a “zoomed” scale value so that the contents of the Block itself is shown zoomed in. Still WIP * Updates to target block contents more reliably and to calc entire box model in width * block-preview: render scaled preview in a shadow dom * try: setting styles to shadow dom elements * Revert "try: setting styles to shadow dom elements" This reverts commit ea573f9. The reason being that there is no benefit of using shadow DOM and it introduces additional complexity. * Removes attempt at auto zoom on Block contents After much experimenting we’ve decided that attempting to reliably detect the visual size of the Block’s visual contents is impossible. This is due to inconsistencies in the DOM markup of each Block. See #16113 (comment) * Updates to remove scale by default in favour of opt in via prop As discussed here #16113 (comment), there is little point in having thumbnail preview and large popover preview showing the same thing. Update to introduce a prop to opt in to scaling the preview. By default the preview is no auto-scaled. Apply `true` value to `BlockPreview` to ensure that Block Styles popover preview is scaled to fit. * Adds scaleFactor prop to provide control for non dynamically scaled preview * Adjust scaleFactor for Block Styles preview * Fix to ensure hooks are called unconditionally to obey rules of Hooks See https://reactjs.org/docs/hooks-rules.html * Removes padding and border from preview on designer advice * Only apply dimensions to preview content when dynamically scaling * block-preview: a different approach attempt. * Correct typo in util function name * Seek the firstChild of the Block DOM node * Tidy up effect timers and improve comments * Dynamically calculate widths and offsets of preview container Removes hardcoded values. * Updates to account for widths of all Blocks passed into to preview Preivously we only considered a single Block scenario. However, the recent addition of multi block support for Previews means we need to ensure we’re measuring the widths of _all_ Blocks. * Revert class additions to Blocks * Adds optional scope to DOM utils. Use to limit scope in Block width comparison. * Update packages/block-editor/src/components/block-preview/index.js Co-Authored-By: Riad Benguella <[email protected]> * Allow prop based opt out from scaling * Fix to ensure smallest Block is found even if smaller than container Preivously we were default to the container width being the largest item. In fact we always want to determine the largest Block contents element and use that to determine the scale. * Remove unecessary Math.min usage * core/button: adjust dims of button wrapper * apply vertical alignment * set scale factor to 0.9 * rollback testing purpose comment * cpre/button: keep adjusting styles for preview * rename vars and css class names * adjust rebase * restore isScaled factor scale * set the scale adjustment in the proper place * update README file * core/button: tidy edit preview styles * coer/button: tweak button preview * preview-block: ensure make the preview visible. * block-preview: hide dropzone * core/button: adjust preview for thumb and full sizes * core/button: set same width for preview * core/button: because it's !important * core/button: set nowrap button in preview * core/button: set nowrap onlu for button * core/quote: adjust quote size * Make previews overflow to the bottom. Currently still has a bug where the large `.block-editor-block-switcher__preview` pane behaves like it's previewing a taller block, when it's not * Revert "core/button: set nowrap onlu for button" It breaks previews of long buttons This reverts commit 758468c. * Fix unit test * core/button: apply nowrap only to buttons * Try a fixed canvas width * Fix blocks editor styles * core/button: centering preview * core/button: adjust only into teh styles preview * block-preview: remove scaleAdjustment property * block-preview: hide inserte element in preview * block-preview: just pick up the first block to scale * block-preview: fix set tall class. X position (wip) * popover: remove commented lines * Refactor the preview * Remove debug code * simplify preview resets * Fix the preview recomputing * Vertical alignining small blocks * block-editor: restore viewportWidth as a property * Update Readme docs Props @marekhrabe. * Remove tests for now We'll revisit when we have a better idea what specifically we should be testing * block-preview: simplify comparision * Remove readme updates that break tests * Update docs properly. See 6f29c27
@@ -91,3 +75,7 @@ | |||
margin-top: $grid-size-large; | |||
} | |||
} | |||
|
|||
.wp-block-button-wrapper { | |||
display: table; |
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 display: table;
the only way to solve this? It seems like a bit of a hack that'll make life a lot harder in the long run.
// Important to set the origin. | ||
transform-origin: top left; | ||
|
||
// Resetting paddings, margins, and other. |
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'm not sure if it was introduced here, but while the preview shows well in most contexts, in the block transform menu, I see horizontal scroll:
From some inspection, it seems to be because of the negative offsets from pseudo-elements surrounding the block in the block list:
gutenberg/packages/block-editor/src/components/block-list/style.scss
Lines 1120 to 1122 in be671ea
left: -$block-padding * 2; | |
position: absolute; | |
right: -$block-padding * 2; |
I'd think that ideally, we could have some concept of a "read-only" block list rendering, since these previews have no use for all of the extra control elements of a block.
But I comment here because it seems like you had to deal with this anyways. Maybe something we can be addressing with overflow: hidden
?
(Aside: I find it helps to use the "Show Scroll Bars: Always" option in macOS, to help discover these sorts of issues: https://cloudup.com/cqF641QROEe)
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 don't have a good answer here, This was a very complex PR and I know that it can be tricky to get right on all situations.
read-only mode or preview mode
I see this mentioned over and over and I think we'll probably need that for several things (previews and also some special FSE mode where some blocks are "enabled" while others are not)
negative left/right
This looks specific to the hover and selected styles, so yeah if we can move this out of the flow somehow (overflow: hidden maybe but I'd be very cautious), it could work or just remove the selected/hover styles entirely in preview.
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.
👋 Honestly, I don't remember exactly the purposes of introducing these resets here. As Riad mentioned, it's a very complex PR and any modification can affect other instances where the preview is being used. AMost of these resets are usually an attempt to make cross-theme support.
read-only
, view
, etc. have been talked as well. I thought that even we could remove all elements which are not needed just for the preview (appender, drop-zone, etc) in order to make the preview process more efficient.
This PR has been cloned from #16113 in order to achieve the same goal but from a different approach.
In short, the final idea behind of these PRs is improving the visualization of the
<BlockPreview />
.Some screenshots
Preview styles in the sidebar
Full preview
How can use it
Unlike the previous approach, this proposal does not require that the dimensions of the block (
srcWidth
andsrcHeight
), the scale factor be defined (scaleFactor
), and whether or not the scale should be adjusted (fitToScale
). So, we only need to pass theblocks
property to the<BlockPreview />
component to make it work.How does it work
Width and the Scale.
The
scale
is computed only considering the width of the blocks and its container. We could polish this approach incorporating the possibility to fit the preview in height, centered, etc. I image something like thebackground-size
andbackground-position
.The scaled is computed dividing the width of the container element (it belongs to the
<PreviewBlock />
component) by the width of the block to visualize. VERY IMPORTANT: it works with only one block so far. However, I'm quite confident that we can make it work for more many blocks as well.To get the width of the container we use the
useRef()
hook and theref
property. Kind simple.Getting the width of the block to preview is kinda hacky at the moment. There is something funny when we try to get the DOM element. Although we are using the
useLayoutEffect()
hook, the dom isn't there yet meaning that when we try to pick it up through its clientId (using a dom/util function) it returns null.First hack: setTimeout(). Wrapping the code into this function we can get the DOM element. I'd like to think that we can get a better solution, digging the reason why it happens. It seems to be related to the
<BlockEditorProvider />
block. I've followed advice from @youknowriad, creating a new component to wrap the<BlockList />
but didn't work either. Maybe I'm missing something.Another relevant change that I'm introducing here is using a CSS class to identify what part(elements) of the Block we want to visualize. AFAIK, This is completely needed since we don't have any other way to detect the exact part of the component that we want.
Although the
getBlockDOMNode()
returns the DOM Element of the block container, generally it doesn't match which what we'd like to visualize. And we can't generalize among all blocks saying ... just pick the first child of its children, or something like that.Adding this new
.wp-block-preview-container
to those elements of the blocks, we can run a query selection easily. For this, I've added thegetClockPreviewContainerDOMNode()
function.It means that we need to explicitly add this class the blocks that can be previewed. This PR adds the class to the
table
,button
andpullquote
component. While this seems to be complicated, it could also be considered one that allows us to easily define which elements we want to visualize from the block.Finally, once we have the container width and the block with, the rest is a matter of simple calculation.
How to test
table
,pullquote
and/or abutton
.Status
Pretty sure there are a lot of things to improve/change and at the end, I'm not 100% confident about this approach neither.