BlockPreview: add __experimentalOnReady property #17242

retrofox · 2019-08-28T22:01:41Z

Description

This PR adds a new property called __experimentalOnReady to the <BlockPreview /> component, which allows us to know the exact moment when the preview is ready, providing useful data such as the scale factor applied, position and the reference to the DOM element.

It worths to mention that the whole process of previewing contemplates some tricks, pretty common when it needs to interact with the real DOM tree btw, such as adding a timeout (100ms), which sometimes makes it unpredictable and prone to fall in some race conditions if you aren't being careful.

Also, it adds the __experimentalScalingDelay_ in order to allow set explicitly the delay to take before to start to compute the scale factor. The default value is 100ms.

How it works?

<BlockPreview
	blocks={ blocks }
	__experimentalOnReady={ ( { scale, previewContainerRef, position } ) => {
		console.log( `scale ${ scale } applied to the <${ previewContainerRef.current.tagName }> element.` );
		console.log( `at x: ${ position.x }, y: ${ position.y } position.` );
	} }
/>

How has this been tested?

To test it you can tide a function to this property and confirm that it's called once the preview has been performed, for instance:

<BlockPreview onReady={ console.log } />

Chrome dev console:

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.
I've included developer documentation if appropriate.

jorgefilipecosta

Hi @retrofox, thank you for your contribution things look good I just left some minor comments.
I guess onRead will be useful in some situations but I'm not imagining many possible use cases right now, could you elaborate on the concrete use case for onReady and why it is needed?

packages/block-editor/src/components/block-preview/README.md

jorgefilipecosta · 2019-09-02T17:34:33Z

packages/block-editor/src/components/block-preview/index.js

-				setPreviewScale( containerElementRect.width / viewportWidth );
+				const scale = containerElementRect.width / viewportWidth;
+				setPreviewScale( scale );
+				onReady( { scale, ref: previewRef } );


I'm not sure if we should be passing refs via an event handler it seems to be uncommon, can't we use the normal way of passing references in React? e.g., in this case, BlockPreview may have an optional __experimental ScaledPreviewRef property that is passed to ScaledBlockPreview component which then forwards the ref.

I'm not understanding this feedback, Jorge :-/

can't we use the normal way of passing references in React?

The reference is gotten just here, in this component, and the callback passes the reference as an argument as a chance to get keep working in the preview process if it's needed.

Hi @retrofox I'm suggesting something identical to this commit e7aaee9 where we allow the possibility of the BlockPreview user to pass its own reference, so the event does not need to pass it.

~~Sounds Good 👌 Let me update the PR.~~

Sorry, I think I completely misunderstood the conversation here:

I'm not sure if we should be passing refs via an event handler

It's just another data passed by the callback. The reference is already defined by the Block preview component to be able to compute sizes in order to adjust the scale factor to apply to the preview block.
We could avoid passing the reference if you think it isn't useful.
What we should not do it is create the reference out of the component. It should take over this task.

allow the possibility of the BlockPreview user to pass its own reference, so the event does not need to pass it.

The ScaledBlockPreview component needs to create its own ref in order to allow it access to the DOM of the elements that form the preview because several calculations are required for scaling and positioning...etc.

It looks like you are suggesting we allow the consumer of BlockPreview to optionally pass down a ref which in turn would be passed down to ScaledBlockPreview and ultimately used as the ref here? I guess you could then either utilise the ref provided by the consumer or have one created internally by ScaledBlockPreview although I'm not clear on what use case this would serve.

retrofox · 2019-09-03T23:46:10Z

Thanks @jorgefilipecosta for your comments. Really appreciate it.

could you elaborate on the concrete use case for onReady and why it is needed?

Specifically in our case what we need is to know the scale factor to apply it to other elements/components which are next to the blocks previewed with the <BlockPreview /> component. The <BlockPreview /> component processes the blocks to preview calling a function (setTimeout) asynchronously, which makes almost impossible to know when it's ready.

jorgefilipecosta · 2019-09-20T18:53:32Z

packages/block-editor/src/components/block-preview/index.js

-				setPreviewScale( containerElementRect.width / viewportWidth );
+				const scale = containerElementRect.width / viewportWidth;
+				setPreviewScale( scale );
+				onReady( { scale, ref: previewRef } );


Now we don't need to pass the ref in the event anymore right?

Added a comment here (edited as well, sorry)
#17242 (comment)

getdave

Thanks for working on this @retrofox! This looks go be a good idea. I had a similar thought when I was knee-deep in this component. Not being able to easily tell when the preview is "ready" was a major flaw.

Some minor tweaks here.

@jorgefilipecosta I feel like this entire component has become a lot more complex but we have no test coverage for it. Do you have concerns about this? If so how could one go about writing sane tests for this?

getdave · 2019-09-24T09:10:56Z

packages/block-editor/src/components/block-preview/README.md

+```es6
+
+<BlockPreview
+	__experimentalOnReady={ ( { scale } ) => console.log( `Current preview scale: ${ scale }` )


It would be good to show all the values available to the callback (ie: not just scale).

getdave · 2019-09-24T09:12:49Z

packages/block-editor/src/components/block-preview/index.js

+				__experimentalOnReady( {
+					scale,
+					position: { x: offsetX * scale, y: offsetY },
+					ref: previewRef,


Have you considered using a more descriptive term rather than ref? What is ref to the user? Might we consider previewContainerRef as this is actually what the ref is "attached" to? Just a thought...

getdave · 2019-09-24T09:13:37Z

packages/block-editor/src/components/block-preview/index.js

 			} else {
 				const containerElementRect = containerElement.getBoundingClientRect();
-				setPreviewScale( containerElementRect.width / viewportWidth );
+				const scale = containerElementRect.width / viewportWidth;


How come this code got added? Why was it added and what does it do? Doesn't seem directly connected to having a callback.

Yes, it does. I've declared the scale constant in order to use it two lines below when it calls the callback.

__experimentalOnReady( { scale, ref: previewRef } );

jorgefilipecosta

Thank you for the iterations @retrofox. This PR seems mostly ready I just left some minor comments.

jorgefilipecosta · 2019-09-30T14:01:42Z

packages/block-editor/src/components/block-preview/index.js

@@ -18,7 +18,15 @@ import BlockEditorProvider from '../provider';
 import BlockList from '../block-list';
 import { getBlockPreviewContainerDOMNode } from '../../utils/dom';

-function ScaledBlockPreview( { blocks, viewportWidth } ) {
+const getOnlineStyles = ( scale, x, y, isReady, width ) => ( {


getOnlineStyles = getInlineStyles?

jorgefilipecosta · 2019-09-30T14:05:12Z

packages/block-editor/src/components/block-preview/README.md

+* `scale`: the scale factor
+* `position`: offsets position (x, y)
+* `previewContainerRef`: DOM element reference
+* `styles`: Online styles applied to the preview container


Did you mean "Inline styles applied.."?
Should the property be called inlineStyles instead of styles to be more specific?

jorgefilipecosta · 2019-09-30T14:08:41Z

packages/block-editor/src/components/block-preview/index.js

@@ -98,7 +112,7 @@ function ScaledBlockPreview( { blocks, viewportWidth } ) {
 	);
 }

-export function BlockPreview( { blocks, viewportWidth = 700, settings } ) {
+export function BlockPreview( { blocks, viewportWidth = 700, settings, __experimentalOnReady = noop, __experimentalDelay = 100 } ) {


The delay we have is for the addition of scaling right? Maybe we can call the prop __experimentalScallingDelay to be more specific I guess ideally the name would refer to what is being delayed.
I think we should also document this prop together with __experimentalOnReady.

with only one l ;-)

getdave · 2019-10-02T11:59:26Z

packages/block-editor/src/components/block-preview/README.md

+* **Type** `Int`
+* **Default** `100ms`
+
+It defines the delay before to start to calc the scale factor and position of the preview block.


How about

Defines a delay to be applied before calculating the scale factor and position of the preview block.

jorgefilipecosta

Thank you for the iterations @retrofox, I tested the new props and they seem to work as expected. The code also looks good 👍

retrofox · 2019-10-14T15:55:16Z

Thank you for the iterations @retrofox, I tested the new props and they seem to work as expected. The code also looks good 👍

Thank you! really appreciate your help reviewing and developing the code. 🙇
Oh, rebase is needed!!!

jorgefilipecosta · 2019-12-18T19:02:00Z

Hi @retrofox, was there an alternative to __experimentalOnReady found? If not, should the PR be rebased and merged?

retrofox · 2020-01-06T18:54:50Z

Hi @retrofox, was there an alternative to __experimentalOnReady found? If not, should the PR be rebased and merged?

Hi @jorgefilipecosta, I'm not aware of an alternative to this approach. PR is already rebased.

The docs say to use run `npm run lint:fix` to automatically fix some issues, that doesn't exist, it should be `npm run lint-js:fix`

github-actions · 2020-02-23T18:19:32Z

Size Change: +136 B (0%)

Total Size: 864 kB

Filename	Size	Change
`build/block-editor/index.js`	104 kB	+136 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.01 kB	0 B
`build/annotations/index.js`	3.43 kB	0 B
`build/api-fetch/index.js`	3.39 kB	0 B
`build/autop/index.js`	2.58 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.02 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	760 B	0 B
`build/block-editor/style-rtl.css`	9.78 kB	0 B
`build/block-editor/style.css`	9.77 kB	0 B
`build/block-library/editor-rtl.css`	7.67 kB	0 B
`build/block-library/editor.css`	7.67 kB	0 B
`build/block-library/index.js`	114 kB	0 B
`build/block-library/style-rtl.css`	7.47 kB	0 B
`build/block-library/style.css`	7.48 kB	0 B
`build/block-library/theme-rtl.css`	669 B	0 B
`build/block-library/theme.css`	671 B	0 B
`build/block-serialization-default-parser/index.js`	1.65 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	57.6 kB	0 B
`build/components/index.js`	190 kB	0 B
`build/components/style-rtl.css`	16.1 kB	0 B
`build/components/style.css`	16 kB	0 B
`build/compose/index.js`	5.76 kB	0 B
`build/core-data/index.js`	10.5 kB	0 B
`build/data-controls/index.js`	1.04 kB	0 B
`build/data/index.js`	8.22 kB	0 B
`build/date/index.js`	5.36 kB	0 B
`build/deprecated/index.js`	771 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.06 kB	0 B
`build/edit-post/index.js`	90.7 kB	0 B
`build/edit-post/style-rtl.css`	8.7 kB	0 B
`build/edit-post/style.css`	8.69 kB	0 B
`build/edit-site/index.js`	4.58 kB	0 B
`build/edit-site/style-rtl.css`	2.77 kB	0 B
`build/edit-site/style.css`	2.76 kB	0 B
`build/edit-widgets/index.js`	4.36 kB	0 B
`build/edit-widgets/style-rtl.css`	2.8 kB	0 B
`build/edit-widgets/style.css`	2.79 kB	0 B
`build/editor/editor-styles-rtl.css`	327 B	0 B
`build/editor/editor-styles.css`	328 B	0 B
`build/editor/index.js`	45.1 kB	0 B
`build/editor/style-rtl.css`	4.13 kB	0 B
`build/editor/style.css`	4.11 kB	0 B
`build/element/index.js`	4.45 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.6 kB	0 B
`build/format-library/style-rtl.css`	500 B	0 B
`build/format-library/style.css`	501 B	0 B
`build/hooks/index.js`	1.92 kB	0 B
`build/html-entities/index.js`	621 B	0 B
`build/i18n/index.js`	3.45 kB	0 B
`build/is-shallow-equal/index.js`	711 B	0 B
`build/keyboard-shortcuts/index.js`	2.3 kB	0 B
`build/keycodes/index.js`	1.68 kB	0 B
`build/list-reusable-blocks/index.js`	2.99 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	215 B	0 B
`build/list-reusable-blocks/style.css`	216 B	0 B
`build/media-utils/index.js`	4.85 kB	0 B
`build/notices/index.js`	1.57 kB	0 B
`build/nux/index.js`	3.02 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.54 kB	0 B
`build/primitives/index.js`	1.49 kB	0 B
`build/priority-queue/index.js`	878 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.3 kB	0 B
`build/server-side-render/index.js`	2.54 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/url/index.js`	4 kB	0 B
`build/viewport/index.js`	1.61 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.18 kB	0 B

_{compressed-size-action}

retrofox requested review from ellatrix, talldan and youknowriad as code owners August 28, 2019 22:01

retrofox requested review from marekhrabe and getdave August 28, 2019 22:01

retrofox mentioned this pull request Aug 28, 2019

SPT: add template title in large preview Automattic/wp-calypso#35840

Merged

retrofox changed the title ~~Update/block preview on ready~~ Update/block preview onReady property Aug 29, 2019

jorgefilipecosta reviewed Sep 2, 2019

View reviewed changes

retrofox force-pushed the update/block-preview-on-ready branch from 73fc6b5 to d96f345 Compare September 4, 2019 14:24

retrofox changed the title ~~Update/block preview onReady property~~ BlockPreview: add __experimentalOnReady property Sep 4, 2019

retrofox added [Type] Enhancement A suggestion for improvement. Needs Dev Note Requires a developer note for a major WordPress release cycle labels Sep 4, 2019

retrofox force-pushed the update/block-preview-on-ready branch from a71db9a to a858784 Compare September 20, 2019 13:42

jorgefilipecosta reviewed Sep 20, 2019

View reviewed changes

retrofox force-pushed the update/block-preview-on-ready branch from a858784 to 1bb0ccf Compare September 23, 2019 15:00

obenland changed the title ~~BlockPreview: add __experimentalOnReady property~~ (1P) BlockPreview: add __experimentalOnReady property Sep 23, 2019

getdave reviewed Sep 24, 2019

View reviewed changes

retrofox changed the title ~~(1P) BlockPreview: add __experimentalOnReady property~~ BlockPreview: add __experimentalOnReady property Sep 24, 2019

retrofox mentioned this pull request Sep 24, 2019

(1P) SPT: set template title using __experimentalOnReady property Automattic/wp-calypso#36264

Closed

retrofox force-pushed the update/block-preview-on-ready branch from 0043103 to c550113 Compare September 24, 2019 15:05

retrofox mentioned this pull request Sep 24, 2019

SPT: use <BlockPreview /> onReady() callback to set template title Automattic/wp-calypso#36269

Closed

jorgefilipecosta reviewed Sep 30, 2019

View reviewed changes

retrofox mentioned this pull request Sep 30, 2019

SPT: immediately render modal with empty thumbnails Automattic/wp-calypso#35713

Closed

getdave reviewed Oct 2, 2019

View reviewed changes

jorgefilipecosta approved these changes Oct 14, 2019

View reviewed changes

retrofox force-pushed the update/block-preview-on-ready branch from 8d6ccbe to cf377c3 Compare January 6, 2020 18:51

brentswisher and others added 15 commits February 22, 2020 09:32

Docs: Fix JavaScript linting command (#20371)

1228391

The docs say to use run `npm run lint:fix` to automatically fix some issues, that doesn't exist, it should be `npm run lint-js:fix`

block-preview: update doc

21f24ef

block-preview: set onReady as experimental prop

c774262

rename reference name

c769593

declare position constant

13d04d4

improve doc

bf3d4c5

use onReady name internally

ca4385b

simply running onReady callback

8453d06

pass the online styles to the callback

6c1f712

avoid var name collisions

c427b41

block-preview: reduce the computing delay to 0

2889313

block-preview: be able to set previewing delay

87dec3b

rename function/vars. Improve docs.

7a37ada

block-preview: improve Readme doc

c215107

block-preview: fix x position. simplify.

ff67c2a

retrofox force-pushed the update/block-preview-on-ready branch from ff2d0bc to ff67c2a Compare February 23, 2020 18:14

retrofox requested review from ajitbohra and chrisvanpatten as code owners February 23, 2020 18:14

retrofox merged commit f054565 into master Feb 25, 2020

retrofox deleted the update/block-preview-on-ready branch February 25, 2020 12:23

github-actions bot added this to the Gutenberg 7.7 milestone Feb 25, 2020

youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BlockPreview: add __experimentalOnReady property #17242

BlockPreview: add __experimentalOnReady property #17242

retrofox commented Aug 28, 2019 •

edited

Loading

jorgefilipecosta left a comment

jorgefilipecosta Sep 2, 2019

retrofox Sep 4, 2019

jorgefilipecosta Sep 19, 2019

retrofox Sep 20, 2019 •

edited

Loading

retrofox Sep 23, 2019 •

edited

Loading

getdave Sep 24, 2019 •

edited

Loading

retrofox commented Sep 3, 2019

jorgefilipecosta Sep 20, 2019

retrofox Sep 23, 2019

getdave left a comment

getdave Sep 24, 2019

getdave Sep 24, 2019

getdave Sep 24, 2019

retrofox Sep 24, 2019 •

edited

Loading

jorgefilipecosta left a comment

jorgefilipecosta Sep 30, 2019

retrofox Sep 30, 2019

jorgefilipecosta Sep 30, 2019

jorgefilipecosta Sep 30, 2019

retrofox Sep 30, 2019

getdave Oct 2, 2019

jorgefilipecosta left a comment

retrofox commented Oct 14, 2019 •

edited

Loading

jorgefilipecosta commented Dec 18, 2019

retrofox commented Jan 6, 2020

github-actions bot commented Feb 23, 2020

BlockPreview: add __experimentalOnReady property #17242

BlockPreview: add __experimentalOnReady property #17242

Conversation

retrofox commented Aug 28, 2019 • edited Loading

Description

How it works?

How has this been tested?

Checklist:

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrofox Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

retrofox Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

getdave Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

retrofox commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retrofox Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

retrofox commented Oct 14, 2019 • edited Loading

jorgefilipecosta commented Dec 18, 2019

retrofox commented Jan 6, 2020

github-actions bot commented Feb 23, 2020

retrofox commented Aug 28, 2019 •

edited

Loading

retrofox Sep 20, 2019 •

edited

Loading

retrofox Sep 23, 2019 •

edited

Loading

getdave Sep 24, 2019 •

edited

Loading

retrofox Sep 24, 2019 •

edited

Loading

retrofox commented Oct 14, 2019 •

edited

Loading