Editor Notes Block

[Copons]

Fixes #

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

• If you're an Automattician, include a shortlink to the p2 discussion with Jetpack Product here.

Testing instructions:

• Go to '..'

Proposed changelog entry for your changes:

[noahtallen]

Just so everyone knows, __experimentalBlocks will change to value when WordPress/gutenberg#21368 is merged

[noahtallen]

(because it matches the api of BlockEditorProvider and basically does the same thing under the hood)

[Copons]

Noted, thanks!

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 2, 2020.
Scheduled code freeze: May 26, 2020

Generated by 🚫 dangerJS against 0e43843

[simison]

When this gets in, let's clean out our internal version: https://github.com/Automattic/wp-calypso/tree/master/apps/o2-blocks/src/editor-notes

[simison]

Very cool! I appreciate you've leaned to core styles for this. :-)

[simison]

You might want feature availability check before using the experimental feature. :)

[Copons]

Yup! Actually the idea was to limit this block to a minimum Guten version.

[Copons]

When this gets in, let's clean out our internal version: https://github.com/Automattic/wp-calypso/tree/master/apps/o2-blocks/src/editor-notes

@simison I think the use cases are different (@dmsnell correct me if I'm wrong):

• The existing one is to leave notes inside the post editor, visible to anybody when editing the post.
• This one is to leave notes in the actual post, visible in the front end too assuming the user has enough capabilities.

This said, I totally forgot about that other block, so we'll need to find a different name for this! 🙂

[simison]

The other one is internal and the slug is different (a8c/*) so it might make more sense to rename the internal one if this is better name than alternatives.

[dmsnell]

Thanks @Copons for the ping. You are correct: the a8c/editor-notes does and has existed in order to provide clues to a post's author when things are unfinished or need updating. The iteration path was to provide a step in the publish flow to list the notes and have a checkbox for each note to indicate if it's "handled" or complete; something which you can use to prevent accidentally publishing a post when you wrote "Make this section better before publishing"

It would definitely be nice to have a description on the motivation and intention for this block before discussing renaming a block that already exists and has used the name for over a year. A screenshot could help too!

Are these meant to be asides in text flow to give some context to what is being written?

[Copons]

@dmsnell You're totally right, I should have written something in the PR description, but haven't had the chance yet, as I'm currently doing this in my 1% time. 😄

You can find more context (and a few screens) here: pb5gDS-xd-p2

Basically:
a8c/editor-notes: notes that appear in the post editor
jetpack/editor-notes: notes that appear in the front end for other (user) editors

The idea really resonates with me a lot, thinking of my support rotations.

When frantically browsing en.support to find a good answer to a ticket or chat, it would be extermely helpful to also find a good predef, or some edge cases to consider, etc.

Or, for example, when releasing a new feature, the devs could add notes to its support page, listing some known bugs and workarounds.

[dmsnell]

When frantically browsing en.support to find a good answer to a ticket or chat, it would be extermely helpful to also find a good predef, or some edge cases to consider, etc.
Or, for example, when releasing a new feature, the devs could add notes to its support page, listing some known bugs and workarounds.

This is actually an idea I've had and yeah I want to update a8c/editor-notes to be a dumb container like you have here - <InnerBlocks.Content />

The way you talk about it though sounds more akin to what people do with role and membership plugins. These may not inherently be "editor notes" so much as they are "privileged" or "restricted" content. Do you think it would make sense to be something like that?

There are certain blocks that I have observed take their initial form as a specific solution to a specific problem but which (without any code change) are actually a generic solution to many more problems.

A few thoughts:

• would this feel more broadly usable if it were marketed as restricted content or role-based content? must it be edit_other_posts or could we provide a drop-down for the capability necessary to view the content?
• what happens if I load my own post and an editor/admin has added notes but I don't have edit_other_posts? do we have a good fallback indicating that I'm unable to see the notes?
• must the content be hidden in post meta? if it's not essential to have the top-most security we could store the inner blocks in the post, get rid of the block meta and post meta, then have a render_block() function in PHP which performs the check and drops the note if the requesting viewer doesn't have permission
• if instead of hard-coding edit_other_posts we added that as a default criteria, we could turn this into an interoperable component with almost no code changes. say you can choose edit_other_posts or you can give the content a name for a type of restriction - like registered_users - and then pass that name with the block through a new Jetpack filter. Someone could make a kind of registration plugin with about ten lines of code using the Jetpack block.

<?php
add_filter( 'jetpack_restricted_block_roles', function( $roles ) {
	return array_merge( $roles, [ 'has_active_subscription', 'is_board_member' ] );
} );

add_filter( 'jetpack_restricted_block_rule', function( $allow_block, $block, $role ) {
	if ( false == $allow_block ) {
		return $allow_block;
	}

	$user = wp_get_current_user();
	switch ( $role ) {
		case 'has_active_subscription':
			return $user->is_active;

		case 'is_board_member':
			return in_array( get_board_member_ids(), $user->id, true );

		default:
			return null; // let the next plugin handle it
	}
} );

[Copons]

@dmsnell These are all excellent observations!
I didn't go as far in my exploration, nor I really have time to at the moment, but I can give you the reasoning behind some of my choices.

• I've chosen edit_other_posts (Editors and up) because it's convenient.
  Editors can manage Editor Notes in both their own posts, and other people's.
  Conversely, users without that capability wouldn't be able to edit Editors' posts, removing one layer of complexity.
  Of course I didn't consider that Editors could leave notes in Author's posts, which would require a "plan B", like, showing the block disabled, without content, with a simple label explaining what it is and why it's there.
  Wonder if there's a mechanism to prevent Authors removing the block, though.

• I dabbled with the idea of having a capabilities/roles selector, but decided to keep it simple for now. (Again, this is totally not part of my team's current focus 😄)

• Saving the note in meta was a nice suggestion by Matias.
  In the off chance the content of the note is sensitive, it's better to avoid saving it in the post content, where it could be retrieved in any number of ways.

Of course, feel free to commit and experiment here as you wish!

[stale]

This PR has been marked as stale. This happened because:

• It has been inactive in the past 3 months.
• It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.