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

Annotation API #7718

Merged
merged 20 commits into from
Nov 9, 2018
Merged

Annotation API #7718

merged 20 commits into from
Nov 9, 2018

Conversation

atimmer
Copy link
Member

@atimmer atimmer commented Jul 5, 2018

Description

This PR includes an API to annotation text inside blocks. This PR on its own is mostly useful to plugin authors. This would make it possible for a feature plugin to build #3026. It also allows plugins like Jetpack or Yoast SEO to highlight certain parts of the text to make their feedback more contextual and actionable.

The best way to see this in action is combine it with this Yoast SEO PR: Yoast/wordpress-seo#10265

XPath has been chosen because it is a format that is compatible with the W3C annotation standard. This will give it future compatibility with front-end annotations. Partial XPaths can really easily be combined into a fully qualified XPath that works globally on a certain page. XPath is also the only format in the W3C annotation standard that can refer to text node. This is in contrast to the CSS selector which can't refer to text nodes.

All annotations in the editor get an annotation-text and an annotation-text-[source] class to they can be styled by the source of the annotation.

Browser compatibility

document.evaluate is not supported in all browsers, but for this version of the PR I don't use document.evaluate. I parse the XPath myself to result in a position within the data structure for rich-text.

Doesn't include, but should be added in a later iteration

  • A custom block should be able to track which RichText an annotation is for. Currently this implementation only works for block with a single RichText.
  • More data can be added to an annotation such as comment, author, etc.

How has this been tested?

Screenshots

annotation poc

Related

Commenting API: #3026.
REST API for annotations (required for Commenting API): #4685

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@atimmer atimmer force-pushed the try/annotations branch 2 times, most recently from d670f2b to e100b06 Compare August 3, 2018 11:28
@atimmer atimmer requested review from aduth, gziolo and youknowriad August 3, 2018 13:05
@gziolo
Copy link
Member

gziolo commented Aug 6, 2018

It looks like we should find a good solution for making RichText extensible so this could be developed as a plugin initially to speed up things. Related issues: #3688 and #4658. /cc @iseulde

Screencast looks really nice 👍

@gziolo gziolo requested a review from ellatrix August 6, 2018 14:35
@gziolo gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible labels Aug 6, 2018
@gziolo
Copy link
Member

gziolo commented Aug 22, 2018

@iseulde - is there any way we could allow register custom TinyMCE plugins? It looks like this is the blocker for having annotations developed as a plugin. Another question is if we want to have it included in the core at some point.

@ellatrix
Copy link
Member

With the RichText state PR (#7890) and future helpers build on that, it should no longer be necessary to do this as a TinyMCE plugin, and then it can also easily be made as a plugin for Gutenberg.

@atimmer
Copy link
Member Author

atimmer commented Sep 12, 2018

An alternative implementation of this PR can be found in the try/annotations-with-rich-text-refactor branch. That branch is based on #7890 and I've updated Yoast/wordpress-seo#10265 to work with that branch.

Whenever #7890 is merged I will make try/annotations work with the new rich text structure.

@atimmer atimmer changed the title Try an annotation API Annotation API Oct 2, 2018
@ellatrix
Copy link
Member

ellatrix commented Oct 4, 2018

Could you write some docs as part of this PR that explain how to use the annotations API?

It is not possible to have overlapping annotations.

Could you elaborate? Why not?

@atimmer
Copy link
Member Author

atimmer commented Oct 4, 2018

Could you elaborate? Why not?

Because of the new RichText structure this is no longer the case. The TinyMCE plugin that I used for matching XPaths couldn't do it (yet).

Could you write some docs as part of this PR that explain how to use the annotations API?

Will do.

@gziolo gziolo added this to the 4.1 milestone Oct 10, 2018
@gziolo
Copy link
Member

gziolo commented Oct 10, 2018

I'm adding this PR to 4.1 milestone to ensure it is included early in the process. As discussed during JS weekly chat yesterday, it depends on #10068 which @iseulde is going to work on as soon as 4.0-RC is out. Those two task depend on each other, so let's make sure we shape formatting API for RichText in a way which makes this PR work :)

@gziolo
Copy link
Member

gziolo commented Oct 10, 2018

There is work in progress PR for formatting API #10209, @atimmer we would love to hear your feedback early on 🙇

@gziolo gziolo added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Oct 10, 2018
@gziolo gziolo modified the milestones: 4.1, 4.2 Oct 15, 2018
@gziolo
Copy link
Member

gziolo commented Oct 15, 2018

I'm moving it to 4.2 as we decided that 4.1 should be UI freeze focused. 4.2 is planned to be focused on API freeze and this PR fits perfectly to that description :)

packages/annotations/README.md Outdated Show resolved Hide resolved
lib/client-assets.php Outdated Show resolved Hide resolved
test/e2e/test-plugins/plugins-api/sidebar.js Show resolved Hide resolved
} );

it( 'allows an annotation to be removed', () => {
const state = annotations( {
Copy link
Contributor

Choose a reason for hiding this comment

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

A good enhancement to these tests would be to apply deepFreeze on the original states. (also need a dev dependency)

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM overall 👍 Thanks for your patience @atimmer

Anyone for a quick second PR as this is a big addition.

packages/annotations/src/store/actions.js Show resolved Hide resolved
selector,
};

if ( selector === 'range' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the selector argument redundant if you also have to provide a range value for it to be used?

In other words, can't we infer selector === 'range' by the presence of a truthy range ?

packages/annotations/src/store/actions.js Show resolved Hide resolved
switch ( action.type ) {
case 'ANNOTATION_ADD':
const blockClientId = action.blockClientId;
const newAnnotation = {
Copy link
Member

Choose a reason for hiding this comment

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

If the action was shaped such that the annotation itself was an object value, we wouldn't have to reconstruct what essentially amounts to const newAnnotation = omit( action, 'type' );

e.g. something like:

action = { type: 'ANNOTATION_ADD', annotation: { id, /* .. */ } };

...state.all,
newAnnotation,
],
byBlockClientId: {
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant data?

getAnnotationsByBlockClientId = ( state, blockClientId ) => filter( state.annotations.all, { blockClientId } );

https://github.com/reduxjs/redux/blob/master/docs/faq/OrganizingState.md#how-do-i-organize-nested-or-duplicate-data-in-my-state

@@ -839,25 +839,25 @@ export class RichText extends Component {
} ).body.innerHTML;
}

valueToFormat( { formats, text } ) {
valueToFormat( value ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Where's the unit test which would have failed under the previous implementation?


const { name, ...settings } = annotation;

registerFormatType( name, settings );
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely related to this pull request, but in mind of #11611, there's an inconsistency we're considering in how / when formats become registered, particularly for "core" formats not specifically registered within @wordpress/format-library.

*/

export function applyAnnotations( record, annotations = [] ) {
annotations.forEach( ( annotation ) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a perfect use-case for Array#reduce:

return annotations.reduce( ( recordResult, annotation ) => {
	// ...

	return applyFormat(
			recordResult,
			{ type: 'core/annotation', attributes: { className } },
			start,
			end
		);
}, record );

Copy link
Member

Choose a reason for hiding this comment

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

I'd do the refactoring but there aren't any unit tests to verify the behavior so I don't feel confident.

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.

Blocking items resolved. Consider unresolved conversations for subsequent pull requests.

@youknowriad youknowriad merged commit 52a3375 into master Nov 9, 2018
@youknowriad youknowriad deleted the try/annotations branch November 9, 2018 17:44
@omarreiss
Copy link
Member

<3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Annotations Adding annotation functionality [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants