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

Allow editing actual posts and pages #356

Merged
merged 4 commits into from
Apr 3, 2017
Merged

Conversation

nylen
Copy link
Member

@nylen nylen commented Mar 30, 2017

This PR builds the initial flow for editing actual posts and pages in the Gutenberg editor. Until the editor is further along, the link to Gutenberg is an additional link on the posts/pages screens rather than a replacement for the default editor:

Much like the built-in "Quick Edit" link, this link is hidden if JavaScript is disabled.

The PR works as follows:

  • Restructure the editor initialization code to accept a post object in the format returned by the WP REST API, with the expectation that we will use this API to build out the editor.
  • If a ?post_id=X value is set, perform an internal (server-side) WP REST API request to load the post with id X. If that succeeds, pass this data to the editor; otherwise use the current "dummy" post content.
  • Add links to the Edit screens for all post types to edit posts/pages in the Gutenberg editor, using the new post_id parameter.

This works well in my testing; however, in order to see post content in the editor, you will currently need to make sure that your post content has some block comment delimiters. For example:

<!-- wp:core/text -->
Welcome to WordPress. This is your first post. Edit or delete it, then start writing!
<!-- /wp:core/text -->

Result:

2017-03-30t07 50 19-0700

array_slice( $actions, 0, $edit_offset + 1 ),
array( 'gutenberg hide-if-no-js' => $gutenberg_action ),
array_slice( $actions, $edit_offset + 1 )
);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit annoying, but $actions is an associative array and we need to insert a key into the middle of it. Technique from http://stackoverflow.com/a/9847709/106302.

index.php Outdated
array( 'wp-blocks', 'wp-element', 'gutenberg-content' ),
false, // $ver
true // $in_footer
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here and elsewhere, I've restructured the code in this function to avoid long lines, which are really detrimental to code readability. I'd recommend that we do this throughout the project, and add rule(s) to our linting setup(s) accordingly.

wp_add_inline_script( 'wp-editor', 'wp.editor.createEditorInstance( \'editor\', { content: window.content } );' );

// Styles
wp_enqueue_style( 'wp-editor-font', 'https://fonts.googleapis.com/css?family=Noto+Serif:400,400i,700,700i', false );
Copy link
Member Author

Choose a reason for hiding this comment

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

The third false parameter here was incorrect. This is the $deps parameter, which is set to array() if it's not an array.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Couple minor notes, but LGTM 👍

index.php Outdated
// ...with a real post
wp_add_inline_script(
'wp-editor',
'wp.editor.post = ' . json_encode( $post_to_edit )
Copy link
Member

Choose a reason for hiding this comment

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

The current approach with wp.editor assumes that multiple instances of an editor could be constructed, and the idea of a single wp.editor.post seems at odds with this. Obvious alternatives to me are:

  • Stop supporting multiple instances of editor and refactor functions accordingly to operate off wp.editor.post directly
  • Assign this as a separate global variable like _wpGutenbergPost

Copy link
Member Author

Choose a reason for hiding this comment

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

Why might we want to construct multiple instances of the editor?

Do we want to do that refactoring to a single editor instance now? I'm guessing probably not, constructing an editor without depending on global state seems like a more desirable code structure regardless of how we end up using it.

If we do use a global variable we can use wp_localize_script which seems slightly more preferred for injecting script data, but doesn't support object paths with . in them.

I don't have a strong preference here, but if we're going to change something on this PR, here's what I would do:

  • Rename the current post to _wpGutenbergPost as you suggested
  • Also rename the "default" test post content from window.content to something like _wpGutenbergTestPost (or, be slightly more clever and set this to _wpGutenbergPost if no post is loaded, simplifying the code path for initializing the editor).

Copy link
Member

Choose a reason for hiding this comment

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

Why might we want to construct multiple instances of the editor?

One original consideration was that it simplifies swapping the existing implementation of wp_editor.

The approach you suggest seems reasonable to me.

);
$request->set_param( 'context', 'edit' );
$response = rest_do_request( $request );
if ( 200 === $response->get_status() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Noting that we'll want to better handle the else condition here at some point (not found, not allowed).

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured we weren't ready to do much about this yet. Anything else you'd like to see in this PR?

Choose a reason for hiding this comment

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

Load a placeholder editor-style.css.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joyously I think this should be done in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joyously if this is something you're interested in seeing sooner rather than later, can you please create an issue explaining a bit about what editor-style.css is used for and why we need to support it?

@nylen nylen force-pushed the add/load-real-post-content branch from 90509b2 to d032dc0 Compare March 31, 2017 23:48
@nylen
Copy link
Member Author

nylen commented Mar 31, 2017

Simplified the post initialization logic in d032dc0. In a future PR, I expect that error handling for invalid posts will make use of error responses returned from the REST API, and maybe use a custom error message if e.g. no post is specified.

I'm satisfied with this PR now. Feel free to merge or leave further feedback which I will address after the weekend.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

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

Successfully merging this pull request may close these issues.

3 participants