Extract BlockThemePreviews-related code from the editor package #50863

okmttdhr · 2023-05-23T08:14:59Z

What?

This PR extract BlockThemePreviews-related code from the editor package. Close: Theme Previews: Remove code from the editor package #50177

Why?

BlockThemePreviews is only enabled in the Site Editor. So, it should be in the edit-site package.

Please refer to #50177

How?

I implemented it so that EntitiesSavedStates can be extensible from the parent component and remove the BlockThemePreviews-related code there.

Basically, I just used props to extend the component, but the tricky part is that we need to refer to isDirty in the parent component since we have logic like this;

	let activateSaveLabel;
	if ( isPreviewingTheme() && isDirty ) {
		activateSaveLabel = __( 'Activate & Save' );
	} else if ( isPreviewingTheme() ) {
		activateSaveLabel = __( 'Activate' );
	} else {
		activateSaveLabel = undefined;
	}

In this case, I decided to 3. Lift the states up but what do you think?

1. `useEffect`

My first thought to achieve this is to use useEffect.
The code is like the below, useEffect notifies the state changes to the parent;
146e452#diff-c5815980e7aa19efd287aaa72512569ac7de514d74679af2f0b099f8a9369bc5R146-R153

This code was easy but had the problem of rendering components more than necessary.
c.f. https://react.dev/learn/you-might-not-need-an-effect#notifying-parent-components-about-state-changes

2. Update everything in a single pass

To reduce unnecessary rendering, we can update the states directly instead of useEffect.
https://react.dev/learn/you-might-not-need-an-effect#notifying-parent-components-about-state-changes:~:text=Delete%20the%20Effect%20and%20instead%20update%20the%20state%20of%20both%20components%20within%20the%20same%20event%20handler%3A

In our case, we can update unselectedEntities and dirtyEntityRecords at the following locations and notify to the parent;
https://github.com/WordPress/gutenberg/blob/024d004/packages/editor/src/components/entities-saved-states/index.js#L57
https://github.com/WordPress/gutenberg/blob/024d004/packages/editor/src/components/entities-saved-states/index.js#L128

However, in this case, isDirty must be calculated on the parent side, which may be difficult to maintain if the logic becomes more complex in the future.

    const isDirty = dirtyEntityRecords.length - unselectedEntities.length > 0;
	if ( isPreviewingTheme() && isDirty ) {

3. Lift the states up

Another approach is to lift the states up.
https://react.dev/learn/you-might-not-need-an-effect#passing-data-to-the-parent

I created the custom hook to compute isDirty and the related values. Also, I implemented EntitiesSavedStatesExtensible and EntitiesSavedStates components for the parent that doesn't need isDirty.
54332d2

Testing Instructions

Testing Instructions for Keyboard

Open the Site Editor and add the following to your URL: ?theme_preview=[themePath] where themePath is the relative path to the theme you want to preview (e.g. twentytwentythree).
Change the site and Activate & Save it.

Screenshots or screencast

Screen.Recording.2023-05-25.at.14.04.55.2.480.mov

github-actions · 2023-05-23T08:15:23Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @okmttdhr! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

packages/edit-site/src/components/save-panel/index.js

getdave

Thanks for the PR.

My initial question here is why are we using context? What's the benefit?

Could we not just pass props directly to the <EntitiesSavedStates> component and control it's behaviour that way?

Maybe I missed something in prior conversations so please feel free to put me right if I've misunderstood.

Update: just noticed this in the description:

Another approach is to use props and just pass them. I'll write the pros/cons later.

I think it would be valuable to consider this tradeoff upfront before proceeding?

getdave · 2023-05-23T10:22:30Z

packages/editor/src/components/entities-saved-states/index.js

+		setIsDirtyContext(
+			dirtyEntityRecords.length - unselectedEntities.length > 0
+		);


Instead of an effect here can we just set the initial value to be this? Generally setState within an effect is a clue that we should be doing this calculation on render.

https://react.dev/learn/you-might-not-need-an-effect

@getdave CC: @scruffian
Thank you for your comment!
I lifted states up and added how I did it in the PR description.
I'll add Testing Instructions/Screenshots and look into the failing tests tomorrow if the implementation makes sense.

packages/editor/src/components/entities-saved-states/index.js

scruffian · 2023-05-23T10:38:34Z

My initial question here is why are we using context? What's the benefit?

That's probably my fault. I know in the past we've tried to avoid too much prop drilling so I suggested context as an alternative, but since we don't need to pass the props through multiple components it might be overkill in this case.

okmttdhr · 2023-05-24T01:31:30Z

@getdave
Thanks for your early review!
I thought there were some use cases to pass the value from a distant parent, as @scruffian said.

After reading your comments, I'll go for using props this time!

getdave · 2023-05-24T08:02:59Z

I thought there were some use cases to pass the value from a distant parent, as @scruffian said.

Do you have any examples of this currently? If not then I wouldn't over optimise for that.

packages/editor/src/components/entities-saved-states/index.js

packages/edit-site/src/components/save-panel/index.js

scruffian · 2023-05-24T09:58:35Z

I've given this a good test and it seems to work as expected, great job. I think if we can just take care of the two points above we should be good to go :)

scruffian · 2023-05-24T10:00:07Z

Oh I also noticed that there are some test failures so we should also look at them.

okmttdhr · 2023-05-25T00:44:03Z

I thought there were some use cases to pass the value from a distant parent, as @scruffian said.

Do you have any examples of this currently? If not then I wouldn't over optimise for that.

@getdave
I meant to say here that at first I thought there might be that use case in some place I didn't know, but after reading your comments, I think that's unlikely, so I'd go for using props!

getdave

Thanks for continuing to work on this. I made some suggestions.

packages/editor/src/components/entities-saved-states/index.js

getdave · 2023-05-25T08:11:25Z

lib/experiments-page.php

-	add_settings_field(
-		'gutenberg-theme-previews',
-		__( 'Block Theme Previews', 'gutenberg' ),
-		'gutenberg_display_experiment_field',
-		'gutenberg-experiments',
-		'gutenberg_experiments_section',
-		array(
-			'label' => __( 'Enable Block Theme Previews', 'gutenberg' ),
-			'id'    => 'gutenberg-theme-previews',
-		)
-	);
-


I would be better to keep this PR surface area small and not remove the experiment in this PR.

Why? Because if we decide we need to roll back to making this an experiment we'll also have to revert all the other code changes. If there have been other changes to those files since then, the unfortunate developer will have to handle that which isn't something we should impose upon them.

Let's make the removal of the experiment a dedicated follow up PR.

I'm going to create another PR for 1f8961a 👍 (reverted it for this PR)
c.f. #50863 (comment)

This reverts commit 1f8961a.

scruffian · 2023-05-25T15:16:18Z

packages/edit-site/src/components/save-panel/index.js

+
+const { EntitiesSavedStatesExtensible } = unlock( privateApis );
+
+const EntitiesSavedStatesForPreview = ( { onClose } ) => {


This is nice. I would have just used an if statement for all this, but I really like the way you encapsulated this logic in a component.

scruffian

Thanks for all your work on this, you've done a great job. LGTM

github-actions · 2023-05-25T15:27:26Z

Congratulations on your first merged pull request, @okmttdhr! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

jeffpaul · 2023-08-02T20:04:02Z

👋🏼 Hey @okmttdhr! If you have a moment, could you connect your GitHub account to your wordpress.org one? This change is slated to be included in the upcoming WordPress 6.3 release on August 8th, and it would be great if we could give you attribution by mentioning you on the credits screen. You can find more details on how contributions are tracked and how connecting GitHub accounts helps in this post on the Make WordPress Core blog.

okmttdhr · 2023-08-07T07:49:33Z

Hello, @jeffpaul 👋 I connected my GitHub account to wporg. Thanks for letting me know!

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label May 23, 2023

scruffian reviewed May 23, 2023

View reviewed changes

packages/edit-site/src/components/save-panel/index.js Outdated Show resolved Hide resolved

getdave reviewed May 23, 2023

View reviewed changes

scruffian reviewed May 24, 2023

View reviewed changes

packages/editor/src/components/entities-saved-states/index.js Show resolved Hide resolved

scruffian reviewed May 24, 2023

View reviewed changes

packages/edit-site/src/components/save-panel/index.js Outdated Show resolved Hide resolved

okmttdhr added 7 commits May 25, 2023 10:08

Extract BlockThemePreviews-related code from the editor package

ca67f2b

Use props to pass values

dabe20e

Lift up the states

59f0b06

Reuse existing functions

b6c20af

extract _EntitiesSavedStates component

0ed41e4

Make EntitiesSavedStatesExtensible provate API

023e17b

Remove window.__experimentalEnableThemePreviews

415c7fb

okmttdhr force-pushed the update/extract-block-theme-previews-to-edit-sites branch from 2004a51 to 415c7fb Compare May 25, 2023 01:40

okmttdhr added 2 commits May 25, 2023 11:15

Remove the experiment option

1f8961a

Fix missing props

2afc222

okmttdhr marked this pull request as ready for review May 25, 2023 05:20

okmttdhr requested a review from spacedmonkey as a code owner May 25, 2023 05:20

okmttdhr requested review from scruffian and getdave May 25, 2023 05:20

getdave reviewed May 25, 2023

View reviewed changes

okmttdhr added 2 commits May 25, 2023 17:25

Revert "Remove the experiment option"

457a357

This reverts commit 1f8961a.

Remove unnecessary destructuring assignment

032a8b6

okmttdhr requested a review from getdave May 25, 2023 08:34

scruffian reviewed May 25, 2023

View reviewed changes

scruffian approved these changes May 25, 2023

View reviewed changes

scruffian merged commit 1100545 into WordPress:trunk May 25, 2023

github-actions bot added this to the Gutenberg 16.0 milestone May 25, 2023

This was referenced May 26, 2023

Remove the experiment option for Block Theme Previews #50983

Merged

Add tests to useIsDirty #51036

Merged

ndiego added [Type] Enhancement A suggestion for improvement. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. labels Jun 7, 2023

Mamaduka mentioned this pull request Jul 17, 2023

Site Editor: Fix incorrect 'useSelect' usage #52683

Merged

okmttdhr deleted the update/extract-block-theme-previews-to-edit-sites branch August 7, 2023 07:49

Mamaduka mentioned this pull request Aug 17, 2023

Editor: Fix the 'useSelect' warning in the 'useIsDirty' hook #53759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract BlockThemePreviews-related code from the editor package #50863

Extract BlockThemePreviews-related code from the editor package #50863

okmttdhr commented May 23, 2023 •

edited

Loading

github-actions bot commented May 23, 2023

getdave left a comment •

edited

Loading

getdave May 23, 2023

okmttdhr May 24, 2023

scruffian commented May 23, 2023

okmttdhr commented May 24, 2023

getdave commented May 24, 2023

scruffian commented May 24, 2023

scruffian commented May 24, 2023

okmttdhr commented May 25, 2023

getdave left a comment

getdave May 25, 2023

okmttdhr May 25, 2023 •

edited

Loading

scruffian May 25, 2023

scruffian left a comment

github-actions bot commented May 25, 2023

jeffpaul commented Aug 2, 2023

okmttdhr commented Aug 7, 2023


		const { EntitiesSavedStatesExtensible } = unlock( privateApis );

		const EntitiesSavedStatesForPreview = ( { onClose } ) => {

Extract BlockThemePreviews-related code from the editor package #50863

Extract BlockThemePreviews-related code from the editor package #50863

Conversation

okmttdhr commented May 23, 2023 • edited Loading

What?

Why?

How?

1. useEffect

2. Update everything in a single pass

3. Lift the states up

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented May 23, 2023

getdave left a comment • edited Loading

Choose a reason for hiding this comment

getdave May 23, 2023

Choose a reason for hiding this comment

okmttdhr May 24, 2023

Choose a reason for hiding this comment

scruffian commented May 23, 2023

okmttdhr commented May 24, 2023

getdave commented May 24, 2023

scruffian commented May 24, 2023

scruffian commented May 24, 2023

okmttdhr commented May 25, 2023

getdave left a comment

Choose a reason for hiding this comment

getdave May 25, 2023

Choose a reason for hiding this comment

okmttdhr May 25, 2023 • edited Loading

Choose a reason for hiding this comment

scruffian May 25, 2023

Choose a reason for hiding this comment

scruffian left a comment

Choose a reason for hiding this comment

github-actions bot commented May 25, 2023

jeffpaul commented Aug 2, 2023

okmttdhr commented Aug 7, 2023

okmttdhr commented May 23, 2023 •

edited

Loading

1. `useEffect`

getdave left a comment •

edited

Loading

okmttdhr May 25, 2023 •

edited

Loading