Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

On creating a new pattern, don't write to the pattern PHP file until you publish #78

Merged
merged 43 commits into from
Mar 3, 2023

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Feb 27, 2023

Thanks to Kameron for this idea.

Fixes GF-3708.

Before

On clicking 'Create New Pattern' in the Patterns UI, it wrote a pattern PHP file to the theme, even before you click 'Update Pattern':

Screenshot 2023-02-28 at 1 53 54 PM

After

This only writes a PHP file to the theme when they click 'Create Pattern' (Publish) in the editor:

Screenshot 2023-02-26 at 11 52 20 PM

This makes the editor more like any other post.

Pattern states

  1. Draft: only saved as an auto-draft post, just like any other post-new.php post
  2. Published: saved to a pattern PHP file

New posts now go to wp-admin/post-new.php?post_type=pm_pattern

This means the editor will show the 'Publish' button (which this renames to 'Create Pattern'):

Screenshot 2023-02-26 at 11 52 20 PM

And after it's published (written to a pattern PHP file), it'll show 'Update Pattern':

Screenshot 2023-02-26 at 11 56 13 PM

Before, there was no notion of publishing, as I sidestepped post-new.php, and simply wrote a new pattern PHP file to the theme.

How to test

  1. Go to the Patterns UI
  2. Click 'Create New Pattern'
  3. Go to the active theme's patterns/ directory
  4. Expected: There's no my-new-pattern*.php file yet:

Screenshot 2023-02-28 at 3 01 19 PM

  1. In the editor, add some content to the pattern and click 'Create Pattern'
  2. Go back to the patterns/ directory
  3. Expected: The pattern PHP file is there:

Screenshot 2023-02-28 at 3 02 27 PM

? get_new_pattern( get_theme_patterns() )['title']
: $post_title;
}
add_filter( 'default_title', __NAMESPACE__ . '\get_default_title', 10, 2 );
Copy link
Contributor Author

@kienstra kienstra Feb 27, 2023

Choose a reason for hiding this comment

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

This is what allows using post-new.php.

We can set the title for new posts on post-new.php:

Screenshot 2023-02-27 at 12 07 00 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a pattern in post-new.php will not have been published yet (saved as a .php file), there's nowhere else I could find to save the title.

@@ -45,6 +45,7 @@ function register_pattern_post_type() {
'supports' => array(
'editor',
'custom-fields',
'title',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now stores the pattern title in ->post_title.

);
}

if ( 'create-new' === filter_input( INPUT_GET, 'action' ) ) {
Copy link
Contributor Author

@kienstra kienstra Feb 27, 2023

Choose a reason for hiding this comment

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

Now that new posts go to post-new.php, we don't need this hack that created a new post and redirected to post.php.

? array_diff_key( $fields, [ 'post_title' => null ] )
: $fields;
}
add_filter( '_wp_post_revision_fields', __NAMESPACE__ . '\ignore_title_field_in_revisions', 10, 2 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed anymore, as the title is stored in ->post_title

if ( ! $pattern ) {
return;
}
$pattern = get_pattern_by_name( $post->post_name );
Copy link
Contributor Author

@kienstra kienstra Feb 27, 2023

Choose a reason for hiding this comment

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

If it's alright, this now stores the pattern name in $post->post_name, not $post->post_title.

This is so we can store the pattern title in the $post->post_title.

post-new.php needs to accept a post with a $post->post_title, so it can have a title like 'My New Pattern 5'.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I moved to store it in post title was this:
Screen Shot 2023-02-13 at 4 24 52 PM

This might cause errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, is it appending numbers to the end, like my-new-pattern-1-4 instead of my-new-pattern-1?

This should prevent that, but maybe it isn't:

function short_circuit_unique_slug( $slug, $post_ID, $post_status, $post_type, $post_parent, $original_slug ) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh cool! I hadn't noticed that. Should be all good then. Nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow! Good to hear!

@@ -22,3 +24,6 @@ addAction(
'pattern-manager/checkActiveTheme',
receiveActiveTheme
);

// @ts-expect-error the @wordpress/editor store isn't typed.
dispatch( editorStore ).disablePublishSidebar();
Copy link
Contributor Author

@kienstra kienstra Feb 28, 2023

Choose a reason for hiding this comment

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

It'd be better to disable the post publish sidebar with a PHP filter:

Screenshot 2023-02-27 at 6 34 25 PM

But there's no good way to filter it because that preference comes from user meta.

@@ -8,7 +9,7 @@ export default function useSave(
setPatternNames: ( patternNames: Array< Pattern[ 'name' ] > ) => void
) {
const isSavingPost = useSelect( ( select: SelectQuery ) => {
return select( 'core/editor' ).isSavingPost();
return select( editorStore ).isSavingPost();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be consistent with how usePostData calls select( editorStore ) instead of select( 'core/editor' ).

If the store of @wordpress/editor is ever typed, we should get the types from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for a change to this, just me venting :D

I get why this is the way it is, and why it's better. They do it in Gutenberg too. But it's always a bummer when I'm searching through gutenberg code and I see this variable, because now it means I have to hunt for where that vairable was defined to see what the actual store is.

It has its benefits for sure, though I feel it makes the code slightly harder to reason about. But again, don't change it just for my personal feelings :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it's a fair point. We could do without this.

It's added just so we get the TypeScript types from it if it's ever typed. But who knows if it'll be typed.

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'll change this. It's for something that may never happen (them typing @wordpress/editor), and we can change it if it does happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 78616c7.

/* When the save button is clicked, hide the pre-publish panel that pops open, as we handle saving in PM. */
.editor-post-publish-panel {
display: none !important;
}
Copy link
Contributor Author

@kienstra kienstra Feb 28, 2023

Choose a reason for hiding this comment

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

The pre-publish panel is now disabled in index.js.

Because we're using the editor more like a normal post, with post-new.php, we can't only hide it via CSS anymore.

It'll block publishing.

@@ -14,7 +14,7 @@ export type PostMeta = {
};

export type SelectQuery = ( dataStore: string ) => {
getEditedPostAttribute: ( postAttribute: string ) => PostMeta;
getEditedPostAttribute: ( postAttribute: string ) => unknown;
Copy link
Contributor Author

@kienstra kienstra Feb 28, 2023

Choose a reason for hiding this comment

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

It feels wrong doing this, but now that usePostData() calls getEditedPostAttribute() with both 'meta' and 'title', this can't be PostMeta.

The return of usePostData() is typed.

It's not actually the saved post,
it's the edited post.
@kienstra
Copy link
Contributor Author

Ready for review again.

Copy link
Contributor

@dreamwhisper dreamwhisper left a comment

Choose a reason for hiding this comment

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

Nice work!

I'm not able to replicate Mike's bug so I'll approve it, but it would be good to get more eyes before merging.

@kienstra
Copy link
Contributor Author

kienstra commented Mar 1, 2023

Thanks a lot, @dreamwhisper! Really appreciate your review.

@kienstra
Copy link
Contributor Author

kienstra commented Mar 2, 2023

As Michael mentioned, this has a bug where you can't add meta values like Categories to a new pattern.

wp-modules/editor/model.php Outdated Show resolved Hide resolved
@kienstra
Copy link
Contributor Author

kienstra commented Mar 2, 2023

Hi @mike-day,
Do you still see the bug?

For anyone else:

Steps to reproduce

  1. Go to the Patterns UI
  2. Click 'Createa New Pattern'
  3. Add some content
  4. Edit some meta, like 'Categories'
  5. Click 'Create Pattern':

Screenshot 2023-03-02 at 5 43 00 PM

  1. Go to the pattern PHP file
  2. Expected: It has the meta you edited:

Screenshot 2023-03-02 at 5 43 20 PM

@mike-day
Copy link
Contributor

mike-day commented Mar 3, 2023

Hi @mike-day, Do you still see the bug?

Nope, it seems to be resolved! In testing right now, the meta is populating in the file as expected 🎉

I did notice something else with the inserter toggle behavior — refer to #87 for an idea on how to cover.

@kienstra
Copy link
Contributor Author

kienstra commented Mar 3, 2023

Nope, it seems to be resolved! In testing right now, the meta is populating in the file as expected 🎉

Thanks so much for finding it and verifying it again.

Also, thanks a lot for finding the bug in #87 and working on it.

Copy link
Contributor

@mike-day mike-day left a comment

Choose a reason for hiding this comment

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

This is really fantastic work, @kienstra! There are some nice changes here that make the app even more intuitive to use when creating a pattern.

Things are working really great in my local — I think we are good to merge.

@kienstra
Copy link
Contributor Author

kienstra commented Mar 3, 2023

Thanks so much for your testing and fixes, @mike-day!

@kienstra
Copy link
Contributor Author

kienstra commented Mar 3, 2023

@johnstonphilip, any concerns before merging?

@johnstonphilip
Copy link
Contributor

@kienstra All good here!

@kienstra
Copy link
Contributor Author

kienstra commented Mar 3, 2023

Thanks, Phil! Thanks a lot for your review!

@kienstra kienstra merged commit 5e5f69b into main Mar 3, 2023
@kienstra kienstra deleted the update/new-post branch March 3, 2023 18:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants