google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 287 forks source link

Display Idea Hub notice above WP posts list table if there are new or saved ideas #3359

Closed felixarntz closed 3 years ago

felixarntz commented 3 years ago

If there are any new or saved ideas in the Idea Hub module, a corresponding notice that links to the respective area of the Idea Hub widget should be displayed above the WordPress posts list admin table, following the Figma design.


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

Acceptance criteria

Implementation Brief

Test Coverage

Visual Regression Changes

QA Brief

===

Changelog entry

felixarntz commented 3 years ago

@marrrmarrr How should the dismissal for these notices behave? Should they be dismissed forever, should they reappear after a certain timeframe?

marrrmarrr commented 3 years ago

@felixarntz I think it would be good for them to reappear after a while. For example:

LMKWYT.

felixarntz commented 3 years ago

@marrrmarrr SGTM. Updated the ACs with this.

felixarntz commented 3 years ago

There is some overlap here with #3360 potentially, building a common dismissal endpoint that supports both use cases.

felixarntz commented 3 years ago

@asvinb A few points of feedback on the IB:

To load dist/js/googlesitekit-idea-hub-notice.js on the WordPress posts list admin table via the admin_enqueue_scripts hook and checking for the appropriate page the script should be loaded via the get_current_screen function and checking the id.

This is subject to change, depending on the decision and IB for #3272. Based on the recent comments there, I assume the approach will be handled more centrally, so we may need to wait for #3272 before finalizing that part in the IB here. In addition, something worth specifying here is what other scripts this will depend on. For example, we need to use the dismissItem action, so we need the asset that includes the core/user store.

The admin_notices hook will be used to display the notifications which should be as per the designs in Figma. The callback function for the hook should do the following:

There has to be additional logic in the admin_notices callback, since the notices should only be rendered when on the posts list screen - not any admin screen (throughout all of which admin_notices fires).

Check if there are saved ideas using the same method as in the Idea_Hub PHP class (still todo as this time of writing)

While the endpoint logic still needs to be adjusted (via #3518), the IB and code for here can already be written without depending on that, since the endpoint is already defined. We need to use GET:saved-ideas and GET:new-ideas from the PHP side here.

  • to include the dismissNotice function with the following details:
    • It has the following parameters:
      • slug: string, unique identifier for the notice
      • expiration_time: integer, having default value 0.
    • Makes a POST request to the core/user/data/dismiss-item endpoint which requires a slug and optional expiration_time.

This doesn't need to be (re-)specified here, it's part of the implementation in #3360. It's sufficient to here only say that the dismissItem action from #3360 will need to be used.

There is one point in the ACs that isn't covered by the IB yet as far as I can tell:

  • The dismissal for the "Saved ideas" notice should be permanent, in general. However, there is one particular circumstance when it should be cleared: If the user makes a change that results in them to no longer have any saved ideas (for example unsaving the only saved idea or creating a draft post from the only saved idea), the dismissal should be cleared. In other words, once the user later saves something again, the notice should re-appear.
asvinb commented 3 years ago

@felixarntz IB updated. Can you check if it's fine to dispatch dismissItem using googlesitekit.data.dispatch( 'core/user' ).

As for this part,

The dismissal for the "Saved ideas" notice should be permanent, in general. However, there is one particular circumstance when it should be cleared: If the user makes a change that results in them to no longer have any saved ideas (for example unsaving the only saved idea or creating a draft post from the only saved idea), the dismissal should be cleared. In other words, once the user later saves something again, the notice should re-appear.

I mentioned it like that:

If there are no saved ideas, clear any dismissed notification using the delete method of the Dismissed_Items class.

felixarntz commented 3 years ago

@asvinb

Can you check if it's fine to dispatch dismissItem using googlesitekit.data.dispatch( 'core/user' ).

This is the right function, but you should be able to access it directly, not via the googlesitekit global - in production build the code gets rewritten anyway to use that global, and the new file here depends on googlesitekit-datastore-user anyway. In other words you should be able to import Data as usual and then use Data.dispatch( CORE_USER ).dismissItem. We only need to rely on the "real" globals in our source code when working with WordPress core-provided APIs (i.e. wp.data....). So this part should be updated.

I mentioned it like that:

If there are no saved ideas, clear any dismissed notification using the delete method of the Dismissed_Items class.

Ah right, sorry I had missed that. That part is good to go then.

The other bit of the IB that now needs updating is how the new JS asset should be loaded. This is based on #3272, so you won't need to modify the Assets class, but instead add the new JS asset to Idea_Hub::setup_assets, using the new admin-posts context described in that issue.

asvinb commented 3 years ago

Thanks @felixarntz . IB updated!

felixarntz commented 3 years ago

@asvinb Excellent - IB ✅

ivankruchkoff commented 3 years ago

Just a small update to IB, rather than using the hooks for _adminnotices we can use includes/Core/Admin/Notice.php and _googlesitekit_adminnotices hook.

ivankruchkoff commented 3 years ago

@felixarntz Figma for the notification for reference, https://www.figma.com/file/YB79eKcGOwyUjlWG8mjDZA/Prompt-Engine-%2F-Idea-Hub?node-id=20332%3A6868

For this implemenation in PHP, we need two SVG images: https://github.com/google/site-kit-wp/blob/develop/assets/svg/logo-g.svg https://github.com/google/site-kit-wp/blob/develop/assets/svg/logo-sitekit.svg

As we don't distribute svg assets with the plugin, that implies creating a class to contain the images, ether creating a combined svg of both, or rendering one and then the other, as an example we have the menu icon in: https://github.com/google/site-kit-wp/blob/develop/includes/Core/Util/Google_Icon.php

Do we want to go ahead and make an update to https://github.com/google/site-kit-wp/blob/develop/includes/Core/Admin/Notice.php to contain a parameter that toggles whether or not a notice has the Google logo and Site Kit image so that we can toggle whether or not a notification is on brand? Or do we prefer that the svg images are rendered just as part of the $content ( https://github.com/google/site-kit-wp/blob/develop/includes/Core/Admin/Notice.php#L52 ).

felixarntz commented 3 years ago

@ivankruchkoff Great point! Let's actually strip these icons entirely and only include the text content - related to the decision that we made in #3272. We can keep these simple, so let's not worry about the SVG icons. Both pieces of copy already clarify in themselves they are related to Site Kit.

I've updated the ACs to mention this differentiation.

eugene-manuilov commented 3 years ago

To summarize: As long as the user continuously has something in "Saved", they will not see any notice again once they dismiss it.

@felixarntz does it mean that if the user has dismissed the "saved ideas" notice and still has saved ideas, we won't need to display neither "saved ideas" nor "new ideas" notices? Or, we should still display the "new ideas" notice regardless of how many saved ideas the user has (one or more)?

felixarntz commented 3 years ago

@eugene-manuilov Yes, the "new ideas" notice should only show up in case the user has no saved ideas. Basically to periodically remind them that there are new ideas, if they are not really actively using Idea Hub.

ivankruchkoff commented 3 years ago

@eugene-manuilov based on the above feedback, I've also added the change to never show the new ideas notice if we have saved ideas present.

wpdarren commented 3 years ago

QA Update: ❌

@ivankruchkoff @eugene-manuilov a few observations:

1) On the Figma designs, there's a Google Site Kit logo, but I am not seeing that on my test site.

image

2) I am also wondering if the QAB is up to date, with the comment left regarding I've also added the change to never show the new ideas notice if we have saved ideas present. From reading the QAB, the same message should appear if there are ideas in New or Saved tabs.

3) Currently, we are unable to pin any ideas so that entries go into the Saved tab, so I am wondering if there's anything we could do on the idea hub connection plugin to fix that so we can test this ticket properly.

eugene-manuilov commented 3 years ago
  1. On the Figma designs, there's a Google Site Kit logo, but I am not seeing that on my test site.

@wpdarren check AC it says except not to include G Site Kit logo for both notifications

  1. Currently, we are unable to pin any ideas so that entries go into the Saved tab, so I am wondering if there's anything we could do on the idea hub connection plugin to fix that so we can test this ticket properly.

Yes, currently, it is not possible to save an idea, therefore we can't really test it yet. We should probably wait till #3747 is implemented to be able to test it.

  1. I am also wondering if the QAB is up to date, with the comment left regarding I've also added the change to never show the new ideas notice if we have saved ideas present. From reading the QAB, the same message should appear if there are ideas in New or Saved tabs.

I have udpated QAB, please, let me know if you still have questions.

wpdarren commented 3 years ago

@eugene-manuilov thank you for updating the QAB and 🤦‍♂️ on the AC! I'll put this on hold until 3747 is in QA.

wpdarren commented 3 years ago

QA Update

Completed the first part of this ticket because we do not need the Idea Hub API to test it.

Noted the presence of a dismissible notice for new ideas. When I dismissed the notice and refresh the page, the notice did not reappear. I reset the dismissed items by running wp user meta delete 1 wp_googlesitekitpersistent_dismissed_items command. Then when the page was refreshed, the dismissible notice is shown again.

image

Have kept this ticket in QA queue until till #3747 is implemented.

wpdarren commented 3 years ago

@eugene-manuilov do you have any updates on when we can test this? :)

eugene-manuilov commented 3 years ago

Unfortunately, not yet, @wpdarren

wpdarren commented 3 years ago

QA Update: Pass ✅

When going to your posts list page after activating the transient, there is a dismissible notice for saved ideas. When the dismiss icon is clicked, and page refreshed the notice does not appear. When I ran the transient again, the notice reappeared when going back to the posts page.