Skip to content
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

Backport block editor persisted preferences #3219

Closed
wants to merge 10 commits into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Sep 9, 2022

Description

Backports the persisted preferences feature from Gutenberg (introduced in WordPress/gutenberg#39795).

Removes the old block editor preferences persistence system.

Testing

  1. Ensure you have the Gutenberg plugin disabled
  2. Open a block editor (it's worth testing each of the post editor, site editor, widgets editor and customize widgets editor).
  3. Change some preferences (e.g. toggle Top Toolbar on)
  4. Clear your local storage data
  5. Reload the page

Expected: Preferences should be retained (e.g. Top Toolbar should still be on) despite local storage being cleared

Trac ticket: https://core.trac.wordpress.org/ticket/56467

Props noisysocks costdev


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@@ -294,6 +294,9 @@ function wp_default_packages_scripts( $scripts ) {
case 'wp-edit-post':
array_push( $dependencies, 'media-models', 'media-views', 'postbox', 'wp-dom-ready' );
break;
case 'wp-preferences':
array_push( $dependencies, 'wp-preferences-persistence' );
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t think this case is necessary as script-loader-packages.php contains everything needed for WordPress to know that wp-preferences depends on wp-preferences-persistence. This switch is for dependencies that can’t be derived from the package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it can be derived from the package.json.

The two packages intentionally have no coupling, as one has WordPress specific code and the other doesn't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I see. And we want it so that when wp-preferences is enqueued it automatically has a persistence layer set up. OK then 👍

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments, this looks good though 👍

'type' => 'object',
'properties' => array(
'_modified' => array(
'description' => __( 'The date and time the preferences were updated.', 'default' ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'description' => __( 'The date and time the preferences were updated.', 'default' ),
'description' => __( 'The date and time the preferences were updated.' ),

@@ -4951,3 +4951,44 @@ function wp_is_application_passwords_available_for_user( $user ) {
*/
return apply_filters( 'wp_is_application_passwords_available_for_user', true, $user );
}

/**
* Register the user meta for persisted preferences.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we give a little context to what "persisted preferences" means? e.g. mention that the block editor uses it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the comment, let me know if you think that looks ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better LGTM 👍

src/wp-includes/script-loader.php Outdated Show resolved Hide resolved
src/wp-includes/script-loader.php Outdated Show resolved Hide resolved
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor things. This could also use tests to protect it going forward.

src/wp-includes/user.php Outdated Show resolved Hide resolved
src/wp-includes/user.php Outdated Show resolved Hide resolved
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
@talldan
Copy link
Contributor Author

talldan commented Sep 13, 2022

Just a few minor things. This could also use tests to protect it going forward.

@costdev Thanks for your review. I'm definitely happy to contribute some tests, but I may need some advice on the best strategy. Is there an easy way to unit test something like the addition of the meta attribute?

@costdev
Copy link
Contributor

costdev commented Sep 13, 2022

@talldan I'd do something like this (untested):

public function test_should_register_persisted_preferences_meta() {
    wp_register_persisted_preferences_meta();

    $actual = get_registered_metadata( 'user', 1, 'wptests_persisted_preferences' );

    $this->assertNotFalse( $actual, 'The expected meta key was not registered' );
    $this->assertSame(
        array(
            // Copy the array contents from source.
        ),
        $actual,
        'The metadata did not have the expected value'
    );
}

P.S. This was typed on mobile, so all indents are spaces and need to be converted to tabs. 🙂

@ironprogrammer
Copy link

Submitted refresh PR including unit test in #3253.

@hellofromtonya
Copy link
Contributor

Closing in favor of PR #3253 which applies this PR and adds tests.

@hellofromtonya
Copy link
Contributor

Newer version in PR 3253 was committed via https://core.trac.wordpress.org/changeset/54182.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants