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

Implement annotations for Gutenberg #10265

Merged
merged 34 commits into from
Nov 12, 2018
Merged

Conversation

atimmer
Copy link
Contributor

@atimmer atimmer commented Jul 5, 2018

Summary

This PR can be summarized in the following changelog entry:

  • Add support for the 'eye' markers in Gutenberg using the experimental annotations API in Gutenberg. They will work for the paragraph, quote, heading and list blocks.

Relevant technical choices:

  • Annotations are available under the wp-annotations handle so I added that as a dependency. To make sure our scripts still load in older versions of Gutenberg I added a no-op script in our WP dependency registration.
  • The text annotations get a annotation-text-[source] class, so I've added a annotation-text-yoast style to get our desired mark color.
  • The eye markers will only be present if the API is present. So in Gutenberg <4.2 the eye markers will simply be hidden. In Gutenberg >4.3 the eye markers will be present and useable.
  • Annotations work for the following blocks: core/quote, core/paragraph, core/list and core/heading.
  • I've implemented adding the annotations with window.requestIdleCallback, this is to prevent freezing the browser while we are adding annotations. This can mean that it takes a bit longer before all annotations are rendered. This is expected behavior. It should never take longer than 1 second per annotation.
  • The calculation for annotations is done by taking the Mark objects and calculating where the the offsets within the block are.
  • I use richText to convert the HTML to be able to purely match the text inside block attributes. This breaks down if there is HTML in our marks, so I strip the marks of HTML. This can be made more sophisticated, but in my testing this was not necessary.

Test instructions

This PR can be tested by following these steps:

Gutenberg <4.2

  • When you open the Gutenberg editor the eye marks should stay hidden.
  • The content analysis should work as expected.

Gutenberg >4.3

This is not yet released and master doesn't have the relevant branch yet until this PR is merged: WordPress/gutenberg#7718. So for now, you need to check out that Gutenberg branch to be able to test this.

  • Open your editor.
  • Type a piece of content that has transition words.
  • See the 'eye' button with the transition word assessment.
  • Click this button.
  • You should see the sentence with the transition word be highlighted.
  • Obviously the editor shouldn't crash.

It should look something like this (I have updated the colors to match the classic editor since then):

annotations

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Fixes #11444

@atimmer atimmer mentioned this pull request Jul 5, 2018
4 tasks
atimmer added 19 commits July 5, 2018 11:36
; Conflicts:
;	js/dist/analysis-80-RC1.js
;	js/dist/analysis-80-RC1.min.js
;	js/dist/commons-80-RC1.js
;	js/dist/commons-80-RC1.min.js
;	js/dist/configuration-wizard-80-RC1.js
;	js/dist/configuration-wizard-80-RC1.min.js
;	js/dist/search-appearance-80-RC1.js
;	js/dist/search-appearance-80-RC1.min.js
;	js/dist/wp-seo-admin-80-RC1.js
;	js/dist/wp-seo-admin-80-RC1.min.js
;	js/dist/wp-seo-admin-global-80-RC1.js
;	js/dist/wp-seo-admin-global-80-RC1.min.js
;	js/dist/wp-seo-admin-gsc-80-RC1.js
;	js/dist/wp-seo-admin-gsc-80-RC1.min.js
;	js/dist/wp-seo-admin-media-80-RC1.js
;	js/dist/wp-seo-admin-media-80-RC1.min.js
;	js/dist/wp-seo-api-80-RC1.js
;	js/dist/wp-seo-api-80-RC1.min.js
;	js/dist/wp-seo-bulk-editor-80-RC1.js
;	js/dist/wp-seo-bulk-editor-80-RC1.min.js
;	js/dist/wp-seo-dashboard-widget-80-RC1.js
;	js/dist/wp-seo-dashboard-widget-80-RC1.min.js
;	js/dist/wp-seo-edit-page-80-RC1.js
;	js/dist/wp-seo-edit-page-80-RC1.min.js
;	js/dist/wp-seo-featured-image-80-RC1.js
;	js/dist/wp-seo-featured-image-80-RC1.min.js
;	js/dist/wp-seo-filter-explanation-80-RC1.js
;	js/dist/wp-seo-filter-explanation-80-RC1.min.js
;	js/dist/wp-seo-help-center-80-RC1.js
;	js/dist/wp-seo-help-center-80-RC1.min.js
;	js/dist/wp-seo-metabox-80-RC1.js
;	js/dist/wp-seo-metabox-80-RC1.min.js
;	js/dist/wp-seo-metabox-category-80-RC1.js
;	js/dist/wp-seo-metabox-category-80-RC1.min.js
;	js/dist/wp-seo-modal-80-RC1.js
;	js/dist/wp-seo-modal-80-RC1.min.js
;	js/dist/wp-seo-network-admin-80-RC1.js
;	js/dist/wp-seo-network-admin-80-RC1.min.js
;	js/dist/wp-seo-post-scraper-80-RC1.js
;	js/dist/wp-seo-post-scraper-80-RC1.min.js
;	js/dist/wp-seo-quick-edit-handler-80-RC1.js
;	js/dist/wp-seo-quick-edit-handler-80-RC1.min.js
;	js/dist/wp-seo-recalculate-80-RC1.js
;	js/dist/wp-seo-recalculate-80-RC1.min.js
;	js/dist/wp-seo-reindex-links-80-RC1.js
;	js/dist/wp-seo-reindex-links-80-RC1.min.js
;	js/dist/wp-seo-replacevar-plugin-80-RC1.js
;	js/dist/wp-seo-replacevar-plugin-80-RC1.min.js
;	js/dist/wp-seo-shortcode-plugin-80-RC1.js
;	js/dist/wp-seo-shortcode-plugin-80-RC1.min.js
;	js/dist/wp-seo-structured-data-blocks-80-RC1.js
;	js/dist/wp-seo-structured-data-blocks-80-RC1.min.js
;	js/dist/wp-seo-term-scraper-80-RC1.js
;	js/dist/wp-seo-term-scraper-80-RC1.min.js
;	js/dist/wp-seo-wp-globals-backport-80-RC1.js
;	js/dist/wp-seo-wp-globals-backport-80-RC1.min.js
;	js/src/edit.js
;	js/src/wp-seo-post-scraper.js
@atimmer atimmer changed the title Implement Gutenberg marking PoC Implement Annotations for Gutenberg Nov 5, 2018
@atimmer atimmer changed the title Implement Annotations for Gutenberg Implement annotations for Gutenberg Nov 5, 2018
@atimmer atimmer changed the base branch from trunk to release/9.2 November 9, 2018 10:52
@IreneStr IreneStr added this to the 9.2 milestone Nov 9, 2018
@designsimply
Copy link

@atimmer I checked https://github.com/Yoast/wordpress-seo/releases/tag/9.2-RC1 and I don't see any mention of the eye markers. Does this mean it's not yet ready for testing in 9.2-RC1 but may be included in an 9.2-RC2 or in the final 9.2 release?

@IreneStr
Copy link
Contributor

@designsimply We're planning on including this PR in at least one RC before the final 9.2 release. The first RC including it will either be RC2 or RC3. You can find the changelog for each RC on our releases page.

The error was "Cannot read property 'getBlocks' of undefined". We need
to actually check if the `core/editor` store is registered before
checking if a certain method is available on that object.
Missing () made the code always try the Gutenberg method for marking.
@afercia
Copy link
Contributor

afercia commented Nov 12, 2018

Worth noting in the Classic Editor I'm getting 2 JS errors:

Uncaught TypeError: Cannot read property 'getBlocks' of undefined
    at isAnnotationAvailable (gutenberg.js?0011:90)

Uncaught ReferenceError: YoastSEO is not defined
    at hasPresenter (getIndicatorForScore.js?bad3:14)

screenshot 2018-11-12 at 10 50 46

@atimmer
Copy link
Contributor Author

atimmer commented Nov 12, 2018

@afercia Should be fixed with
d0eb99d and 8259c5c.

Copy link
Contributor

@IreneStr IreneStr left a comment

Choose a reason for hiding this comment

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

CR 👍 Only some documentation questions/requests. All other issues I found were already fixed in the previous 3 commits.

const startOffset = marked.indexOf( startMark );
let endOffset = marked.indexOf( endMark );

endOffset = endOffset - startMark.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

You are using endOffset in two different 'meanings'* here , which is a little bit confusing.

* offset from beginning of the text, and offset from the beginning of the mark

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be more clear with:

const endOffset = marked.indexOf( endMark ) - startMark.length;

Or:

Suggested change
endOffset = endOffset - startMark.length;
const matchedEndMark = marked.indexOf( endMark );
const endOffset = matchedEndMark - startMark.length;

}

/**
* Calculates an annotation if the given mark is applicable tot he content of a block.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: to the

* So we try to find a mark index based on the mark content with all tags stripped.
*/
if ( foundIndex === -1 ) {
original = mark.getOriginal().replace( /(<([^>]+)>)/ig, "" );
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between stripping all tags using this, and using stripHTMLTags like above? Please comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stripHTMLTags in YoastSEO.js replaces HTML tags with one space, but we want to do another try where we replace HTML tags without replacing them with one space.


const offsets = getOffsets( mark );

const startOffset = foundIndex + offsets.startOffset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to add an offset to the foundIndex? Please add inline comment.

Copy link
Contributor Author

@atimmer atimmer Nov 12, 2018

Choose a reason for hiding this comment

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

The offsets.startOffset and offsets.endOffset are offsets of the <yoastmark> relative to the start of the Mark object. The foundIndex is the index form the start of the RichText until the matched Mark, so to calculate the offset from the RichText to the <yoastmark> we need to add those two indexes/offsets.

Edit: This can probably be refactored to be more readable.

const startOffset = foundIndex + offsets.startOffset;
let endOffset = foundIndex + offsets.endOffset;

// If the marks are at the beginning and the end we can use the length
Copy link
Contributor

Choose a reason for hiding this comment

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

Please turn into multline comment

*
* @param {Paper} paper The paper that the marks are calculated on.
* @param {Mark[]} marks The marks to annotate in the text.
* @returns {void}
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline before returns

* @returns {void}
*/
export function applyAsAnnotations( paper, marks ) {
// Do this always to allow people to select a different eye marker.
Copy link
Contributor

Choose a reason for hiding this comment

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

... while/when another eye marker is still active


// For every block...
const annotations = flatMap( blocks, ( ( block ) => {
// We go through every annotate able attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

annotatable

* @param {string} blockTypeName The name of the block type.
* @returns {string[]} The attributes that we can annotate.
*/
function getAnnotateAbleAttributes( blockTypeName ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Annotatable

*/
initializeAnnotations() {
if ( isAnnotationAvailable() ) {
this._store.dispatch( setMarkerStatus( "enabled" ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this interfere with other places where we dis-/enable the markers? For example, when we detect a pagebuilder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't. We check if the state for annotations is available and If so we put our marks as annotations in the annotation state. This is the most direct feature checking that's possible.

@abotteram
Copy link
Contributor

abotteram commented Nov 12, 2018

Acceptance:

General testing (4.9.8 with Gutenberg plugin):

  • When updating the Keyphrase, the keyphrase markers are not updated.
  • When typing inside a marker, the marker isn't removed, even if it is no longer relevant for the assessment.
  • When collapsing and reopening a readability or seo analysis collapsible, the marker button is disabled, but the markers are still present in the editor. Clicking the marker button again will re-mark the content again.

5.0.0-beta 3 with Gutenberg plugin development version (that should have annotations).

  • Annotations do not work. window.wp.annotations is not available.

No breaking issues. 👍

@abotteram abotteram merged commit b214bb6 into release/9.2 Nov 12, 2018
@abotteram abotteram deleted the stories/gutenberg-annotations branch November 12, 2018 14:49
@klihelp
Copy link

klihelp commented May 13, 2019

Not working on classic editor using Gutenberg Manager plugin.

Ref: #10265 (comment)

Screen Shot 2019-05-13 at 11 49 44

@klihelp
Copy link

klihelp commented May 13, 2019

The above issue is part of the withSelect() Gutenberg 5.6.1 which is not in core yet.
wp.data.select(....) returns null

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.

6 participants