Skip to content

Conversation

@pablinos
Copy link
Contributor

@pablinos pablinos commented Oct 23, 2020

After some discussion, we decided that it was probably beneficial to not
tie the meta key for the last editor used to the one used by the Classic
Editor Plugin. This simplifies the code because we don't need to check
for the plugin being present, and means we can make the meta key hidden
by prepending an underscore, as @jeherve suggested in the original PR.

Changes proposed in this Pull Request:

This PR changes the meta key to _last_editor_used_jetpack and removes the logic to check the Classic Editor Plugin settings if the plugin is active.

There is an associated WPCOM change in D50744-code

Testing instructions:

Create and save a post. Check that it has _last_editer_used_jetpack set as post meta, and that the value is correct - either classic-editor or block-editor

To see the value sync'd, apply D50744-code to your sandbox and set JETPACK__SANDBOX_DOMAIN to an appropriate value.

Proposed changelog entry for your changes:

N/A - this is a tweak to a new feature. The changelog is the same as #17407

@pablinos pablinos self-assigned this Oct 23, 2020
@pablinos pablinos requested a review from a team October 23, 2020 21:22
@pablinos pablinos added the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Oct 23, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented Oct 23, 2020

Warnings
⚠️ The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.
⚠️

pre-commit hook was skipped for one or more commits

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17595

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 6547e5d

@jeherve jeherve added [Pri] Normal [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] WordPress.com Block Editor labels Oct 23, 2020
@jeherve jeherve added this to the 9.1 milestone Oct 23, 2020
Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

A few questions...

Should we add a jetpack prefix to the post_meta key (e.g. _jetpack_last_editor_used) to avoid any potential Core or plugin conflicts?

What do you think about changing Jetpack\ClassicEditor to Jetpack\EditorType, Jetpack\RememberEditor, Jetpack\EditorClient, or similar. I think this would future proof it a little better, in case we add additional clients or expand it to work with the REST API.

I think the function classic_editor_can_edit_post_type from #17407 is actually meant to be block_editor_can_edit_post_type? It looks like it checks the block editor. If so, let's update that in this PR as well.

@pablinos
Copy link
Contributor Author

Should we add a jetpack prefix to the post_meta key (e.g. _jetpack_last_editor_used)

I was thinking about adding jetpack to it, but thought that it's not always Jetpack that's going to be updating it. It lives in this codebase though, so to avoid issues in future, it seems sensible.

What do you think about changing Jetpack\ClassicEditor to Jetpack\EditorType, Jetpack\RememberEditor, Jetpack\EditorClient

Yes, changing ClassicEditor to something more generic makes sense. I'm not so sure about renaming remember_editor as that's what the function does.

I think the function classic_editor_can_edit_post_type from #17407 is actually meant to be block_editor_can_edit_post_type?

This is a hangover from the Classic Editor Plugin - it's namespacing it. Now we have a namespace, I think it can be renamed to can_edit_post_type, or maybe editor_can_edit_post_type.

@pablinos pablinos added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Oct 26, 2020
@pablinos
Copy link
Contributor Author

As we want to try and get this into 9.1, I've moved it on to 'Needs Review'.

@retrofox retrofox requested review from a team October 26, 2020 14:39
retrofox
retrofox previously approved these changes Oct 26, 2020
Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

I've tested the patch and it works as expected. It picks up the current editor and stores it in the post meta key. I suggest holding on to get a 👍 from @Automattic/jetpack-crew folks.

jeherve
jeherve previously approved these changes Oct 26, 2020
After some discussion, we decided that it was probably beneficial to not
tie the meta key for the last editor used to the one used by the Classic
Editor Plugin. This simplifies the code because we don't need to check
for the plugin being present, and means we can make the meta key hidden
by prepending an underscore, as @jeherve suggested in the original PR.
@jeherve jeherve dismissed stale reviews from retrofox and themself via 528ff70 October 26, 2020 17:38
@jeherve jeherve force-pushed the update/editor-meta branch from 0de5e63 to 528ff70 Compare October 26, 2020 17:38
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 26, 2020
jeherve
jeherve previously approved these changes Oct 26, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Approving again after a rebase to fix unrelated test failures.

creativecoder
creativecoder previously approved these changes Oct 26, 2020
Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates @pablinos !

In testing, this saves the last editor loaded for the post in the post meta, as expected.

I did get into a weird state at one point, after first activating the classic plugin for a site, which the value was classic-editor for a post even though I had not yet loaded the classic editor for that particular post. But I couldn't replicate it again, so it may have just been a mistake in my testing.

It does make me wonder if updating the meta on update or publish would be better (though more difficult to achieve, if I recall from your earlier explorations), but that's something we can explore in a future change--this works well enough for what we need.

@pablinos
Copy link
Contributor Author

I did get into a weird state at one point, after first activating the classic plugin for a site, which the value was classic-editor for a post even though I had not yet loaded the classic editor for that particular post

Hmmm, that does sound odd. I did get into an odd state where I went to edit the post using the classic editor and got warned about it having blocks. I clicked to load the block editor by accident and closed the browser. In this situation, the meta had already been updated to classic-editor. Could you have got into some kind of similar state?

It does make me wonder if updating the meta on update or publish would be better (though more difficult to achieve, if I recall from your earlier explorations)

I think we'd be able to update the meta as the post is saved, but Gutenberg will be using the API to do that, so we'd have to pass something through to make sure it's updated. Setting it as the editor is loaded should mean we always get the latest value, and it works the same in the Classic Editor Plugin. Let's see how it goes, and look to update it, if needs be.

@pablinos pablinos dismissed stale reviews from creativecoder and jeherve via 6547e5d October 26, 2020 21:38
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Oct 26, 2020
Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

Re-review with the latest changes using a jurassic.ninja site + the classic editor plugin. All working as expected.

✅ Loading the post with the classic editor gives a meta value of classic-editor
✅ Loading the post with the block editor gives a meta value of block-editor
✅ The meta is synced to wpcom

Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

LGTM

@pablinos pablinos merged commit 4b2d3a0 into master Oct 26, 2020
@pablinos pablinos deleted the update/editor-meta branch October 26, 2020 22:53
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] WordPress.com Block Editor [Pri] Normal [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants