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

Show Idea Hub context for post based on Idea Hub idea in WordPress block editor / Gutenberg #3272

Closed
felixarntz opened this issue May 3, 2021 · 25 comments
Labels
Module: Analytics Google Analytics module related issues P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented May 3, 2021

The WordPress block editor (Gutenberg) should indicate if the current post is a draft based on an Idea Hub idea.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • When editing a WordPress post that has Idea Hub idea metadata set in the block editor (Gutenberg), a notice like in this Figma file should be displayed on top of the editor.
    • The copy used should actually differ from the Figma design and say: This post was created from an idea you picked in Site Kit's Idea Hub: {idea} The bold "Google Site Kit" prefix from the Figma design should not be included.
    • The notice should be dismissible. When dismissed, that dismissal should be cached temporarily for an hour, it doesn't need to be persistently stored. Editing a particular post doesn't happen all the time, and it's okay to remind the user again if they open the same post in the editor after a while.
    • This functionality should rely on the Gutenberg Notice API.
    • The entire integration should be implemented in a separate JS file that is loaded only in the block editor (from the Idea_Hub class in PHP), if necessary for the current post. Other than typical for Site Kit JS code, the code here should actually rely on WordPress's included versions of @wordpress/* dependencies (e.g. reference wp.data instead of @wordpress/data, and include wp-data in the dependencies for the asset in PHP) and not come with any of our own bundled dependencies, to not double-load dependencies and minimize the footprint in the block editor.

Implementation Brief

PHP

  • Edit the Asset class:
    • Add the following constants to the class:
      • CONTEXT_ADMIN_GLOBAL = 'admin-global'
      • CONTEXT_ADMIN_POST_EDITOR = 'admin-post-editor'
      • CONTEXT_ADMIN_POSTS = 'admin-posts'
      • CONTEXT_ADMIN_SITEKIT = 'admin-sitekit'
    • Add 'load_contexts' => array( self::CONTEXT_ADMIN_SITEKIT ), to the array of default values for the $args property.
    • Add a new public method has_context( $context ) that returns TRUE if the incoming $context exists in the $this->args['load_contexts'] array.
  • Update the enqueue_assets method of the Module_With_Assets interface to accept $context = Asset::CONTEXT_ADMIN_SITEKIT argument.
  • Edit the enqueue_assets method of the Module_With_Assets_Trait trait to have the same new $context = Asset::CONTEXT_ADMIN_SITEKIT argument and to filter out assets that don't have provided context before enqueueing assets.
  • Add a new hook for the enqueue_block_editor_assets action in the Assets::register method to enqueue registered gutenberg assets. Use Assets::get_assets() method to return registered assets and filter out assets that don't support the CONTEXT_ADMIN_POST_EDITOR context.
  • Edit the Idea_Hub::setup_assets method to define a new Script for the dist/js/googlesitekit-idea-hub-notice.js with the CONTEXT_ADMIN_POST_EDITOR context and wp-data as a dependency.

JS

  • Create assets/js/googlesitekit-idea-hub-notice.js which has the showIdeaHubNotice function and the file which will be enqueued in the block editor only.

  • Update webpack.config.js to add a new entry for the above file similar to analytics-advanced-tracking.

  • The showIdeaHubNotice function should do the following:

    • Check if the current post has idea hub metadata via the getEditedPostAttribute selector of core/editor data store. As per the IB, wp.data should be used. For e.g:
      const postMeta = wp.data.select( 'core/editor' ).getEditedPostAttribute( 'meta' );
    • The following meta keys should be present and not empty to identify an idea hub post:
      • googlesitekitpersistent_idea_name
      • googlesitekitpersistent_idea_text
      • googlesitekitpersistent_idea_topics
    • Import the setItem and getItem from assets/js/googlesitekit/api/cache.js.
    • Use the getItem function to check if there is an item in local storage with the key modules::idea-hub::dismissed-editor-notice-<post_ID> where <post_ID> is the ID of the currently edited post. To get the current post ID, query the core/editor data store via the getCurrentPostId selector.
    • If cacheHit is false from the result of getItem, then the user has not dismissed the notice. The Gutenberg notice API will be used to display the notice but the onDismiss callback did not work until recently. To circumvent that, we can create listen to data store changes checking whether the notice is still present on the page or has been dismissed.
    • To display the notice:
      • Use the Gutenberg notice API to create an info dismissable notice as per the designs in Figma by using the createInfoNotice action of the core/notices data store. For e.g:
      wp.data.dispatch( 'core/notices' ).createInfoNotice( 'My custom notice', options ); // options is an object
      • The text for the notice should be as per the second bullet point from the AC where {idea} is the value from the post meta with the googlesitekitpersistent_idea_text key.
      • The id of the notice should be googlesitekit-idea-hub-notice.
      • Since we want to know when the user dismisses the notice and the onDismiss property does not work, listen to the data store to see if the notice is still present.
        • Define a function, hasNotice which returns true if the notice id defined in the previous bullet point is in the list of notices. To get the list of notices, use the getNotices selector of the core/notice data store.
        • Define another function, handleDataChange change, which checks whether hasNotice returns true. If this is the case, then the notice is currently being displayed on the page. If hasNotice returns false, this means that the user has dismissed the notice. In that case, do the following:
          • Use the setItem to set the key modules::idea-hub::dismissed-editor-notice-<post_ID> to have true as value.
          • We can only pass the key and value parameters to the setItem function since the default parameters set the expiry for the item in local storage to 1 hour.
        • Listen to the changes to the data store using the subscribe function of wp.data, passing handleDataChange as parameter. The subscribe function returns a unsubscribe function used to stop the subscription. This should occur in the handleDataChange function when hasNotice returns false.
    • If cacheHit is true from the result of getItem, do not do anything. It means that the user has seen the notice and dismissed it in the past hour.
  • POC for listening to data changes: Show Idea Hub context for post based on Idea Hub idea in WordPress block editor / Gutenberg #3272 (comment)

Test Coverage

  • No new tests to be added.

Visual Regression Changes

  • N/A

QA Brief

To the engineer writing the QA Brief: Similar to #3271, this issue should be testable by our QA team and not require QA: Eng. Include a sample datastore call to do in the browser console (e.g. window.googlesitekit.data.dispatch( 'modules/idea-hub' ).createIdeaDraftPost( ... )) so that a draft post is created that can then be checked.

Changelog entry

  • Add the Idea Hub notification to the WordPress block editor
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels May 3, 2021
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz May 3, 2021
@asvinb asvinb self-assigned this May 6, 2021
@asvinb
Copy link
Collaborator

asvinb commented May 6, 2021

@felixarntz @aaemnnosttv A couple of things I caught out when working on this IB:

  1. The Google Site Kit text is bold as per the designs, and in order to achieve that, we need to use the unstableHTML property in the options to create a notice.
  2. The Google Site Kit text has some margin right applied to it in the designs. In order to achieve that, either we add some inline styles and wrap the text between span or we add a custom class name and then load a stylesheet which contains the styles for the class name. I think it might be overkilled to do that, and to instead just use the normal spacing between the words and use ":" maybe to separate "Google Site Kit" from the other text.
  3. The background color for the info notice should be blue as per https://developer.wordpress.org/block-editor/reference-guides/components/notice/ and the designs. However when I tested using the latest WordPress release, the background for the info notice was white. Not sure if something has changed and the docs are not up to date.
    image

@asvinb asvinb assigned felixarntz and unassigned asvinb May 6, 2021
@felixarntz
Copy link
Member Author

@asvinb Thanks for raising these points, it leads me to question a little bit the wording we use here in the first place.

@marrrmarrr Is the wording like we have it in this Figma design really what we should have here? The primary concern is that this doesn't use proper language while typical WordPress notices do, i.e. full sentence (e.g. This draft post was created based on the Site Kit idea "..."). Could we adjust this to be 1-2 sentences instead of what it currently is?

@marrrmarrr
Copy link
Collaborator

@felixarntz the wording in Figma is not final, it was purely to help explain the flows, and definitely open to edits.
We could change this to be more descriptive, e.g. "This draft post was created from an idea you picked in Site Kit's Idea Hub: "
We could keep the idea text on a new line for clarity. LMKWYT.

@felixarntz
Copy link
Member Author

@marrrmarrr Sounds good, with just one remark: Let's use just "This post..." instead of "This draft post..." so that it also works for published posts based on an Idea Hub idea.

@asvinb Can you update the relevant bits of the IB accordingly? In the future, to simplify this I'd recommend to not re-phrase exact wording and rather say "the copy from the ACs" or "the copy from the Figma design" or so.

@felixarntz felixarntz assigned asvinb and unassigned felixarntz and marrrmarrr May 14, 2021
@asvinb
Copy link
Collaborator

asvinb commented May 17, 2021

Sounds good @felixarntz . Thanks!

@asvinb asvinb removed their assignment May 17, 2021
@fhollis fhollis modified the milestones: Sprint 49, Sprint 50 May 18, 2021
@eugene-manuilov eugene-manuilov self-assigned this May 18, 2021
@eugene-manuilov
Copy link
Collaborator

Thanks, @asvinb. IB looks solid overall, as usual, but I have two comments for you:

Edit includes/Modules/Idea_Hub.php to load dist/js/googlesitekit-idea-hub-notice.js using the enqueue_block_editor_assets hook and having wp-data as dependency.

I think we need to introduce a new approach of enqueueing Gutenberg-related assets that will follow the similar logic that we use for regular admin assets. Here is what we need to do:

  1. Create a new interface Module_With_Gutenberg_Assets that replicates the Module_With_Assets interface but uses *_gutenberg_assets instead of *_assets method names.
  2. Create a new trait Module_With_Gutenberg_Assets_Trait that replicates the Module_With_Assets_Trait trait and also uses *_gutenberg_assets instead of *_assets method names.
  3. Update the Idea_Hub class to implement the Module_With_Gutenberg_Assets interface and use the Module_With_Gutenberg_Assets_Trait trait.
  4. Add a new protected method setup_gutenberg_assets to the Idea_Hub class which should return an array of Script objects (in our case it will be an array with a single Script for the new googlesitekit-idea-hub-notice.js script).
  5. Add a new private method get_gutenberg_assets to the Assets class that applies a new filter googlesitekit_gutenberg_assets on an empty array and returns returned results from the filter call.
  6. Update the Assets::register_assets method to register gutenberg assets as well using the new get_gutenberg_assets method to get gutenberg scripts.
  7. Add a new hook for the enqueue_block_editor_assets action in the Assets::register method to enqueue registered gutenberg assets.
  8. Add a new hook for the googlesitekit_gutenberg_assets filter that copies the logic used for googlesitekit_assets but calling Gutenberg related methods:
    add_filter(
    'googlesitekit_assets',
    function( $assets ) use ( $available_modules ) {
    foreach ( $available_modules as $module ) {
    if ( $module instanceof Module_With_Assets ) {
    $assets = array_merge( $assets, $module->get_assets() );
    }
    }
    return $assets;
    }
    );

cc @felixarntz @aaemnnosttv in case you want to add or change something

Check if there is the googlesitekit-idea-hub-notice-<post_ID> cookie set via the js-cookie package where <post_ID> is the ID of the currently edited post

Let's use the localStorage instead of cookies.

@felixarntz
Copy link
Member Author

@eugene-manuilov +1 for the suggested approach!

We can later also consider implementing similar infrastructure for any frontend assets (which we'll need for some future functionality that has been discussed in the past).

@aaemnnosttv
Copy link
Collaborator

@eugene-manuilov I'm not sure we should replicate Module_With_Assets with a Gutenberg-specific variant. That would certainly work, but Gutenberg assets are still assets so to me these should be returned with all the other assets. Maybe instead we should add a way to guard (or otherwise control) assets from being enqueued based on the context?

@eugene-manuilov
Copy link
Collaborator

@aaemnnosttv yes, we can add a context ('admin', 'gutenberg', 'frontend') parameter to the get_assets function and return a specific set of assets based on provided context, but I think it would be better if separate concerns because it will be easier to understand, test and maintain.

@asvinb
Copy link
Collaborator

asvinb commented May 20, 2021

@eugene-manuilov IB updated to use localStorage instead of cookies. Once @aaemnnosttv, @felixarntz and you are aligned on the way to enqueue Gutenberg assets, I'll update the IB with your approach.

@tofumatt tofumatt self-assigned this Jun 22, 2021
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jul 2, 2021
@fhollis fhollis modified the milestones: Sprint 52, Sprint 53 Jul 5, 2021
@tofumatt tofumatt assigned eugene-manuilov and unassigned tofumatt Jul 6, 2021
@tofumatt tofumatt assigned eugene-manuilov and unassigned tofumatt Jul 6, 2021
@eugene-manuilov eugene-manuilov removed their assignment Jul 7, 2021
@cole10up cole10up self-assigned this Jul 7, 2021
@cole10up
Copy link

cole10up commented Jul 9, 2021

QA ✅

Confirmed the idea hub banner is displayed on a Gutenberg post in draft mode.

image

Was able to close the banner properly and confirmed the dialogue is correct.

One thing I noticed is the banner doesn't come back if the user closes it, leaves the drafted post without saving any edits then navigates back into the draft.

Moving to approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants