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.25k stars 290 forks source link

Update Blue Box CTA components to more visually-illustrative versions #4694

Closed tofumatt closed 2 years ago

tofumatt commented 2 years ago

Feature Description

The current "Blue Box" CTAs should be replaced as part of the Zero States epics to show a more visually-interesting and evocative CTA. The new style can be seen in the Figma designs. The CTA for connecting Analytics in the Search Funnel widget looks like this, for instance:

CleanShot 2022-01-21 at 01 40 18

This issue should cover updates to all existing "Blue Box" CTAs in one place, using the styles provided in the Figma design.


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

Acceptance criteria

This issue does not cover moving the CTAs below the chart on mobile. That will be covered in #4695, a separate issue that depends on this one.

This issue does not cover Admin Bar or WP Dashboard Blue Box CTAs because we are waiting on design for them. For now, this issue should only address the Analytics CTAs on Site Kit Dashboards.

Implementation Brief

There is a POC, which explores the structure and responsive layout and can be used as a reference/starting point.

Note: After initially implementing according to the brief below, we noticed the responsive behaviour needed addressing, so some additional changes were made. See this comment for details.

Extract reusable CTA click handler into a hook

This will be useful as we migrate more of the CTAs away from ActivateModuleCTA.

Within new file assets/js/hooks/useActivateModuleCallback.js:

Create PreviewGraph component

Within new file assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/PreviewGraph.js:

Create ActivateAnalyticsCTA component

Within new file assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/ActivateAnalyticsCTA.js:

Create CreateGoalCTA component

Within new file assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/CreateGoalCTA.js:

Render the new CTA components

Within file assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/Overview.js:

Test Coverage

QA Brief

Note: This issue does not cover moving the CTAs below the chart on mobile. That will be covered in #4695, a separate issue that depends on this one.

Changelog entry

felixarntz commented 2 years ago

@tofumatt Let's make sure to capture in the ACs here that the new component should be Analytics specific. Right now we have the more generic ActivateModuleCTA component, but that should not be adjusted itself since it's still used in other places. So we should just replace that usage in the one place on the main dashboard/entity dashboard where it is per the Figma design.

@marrrmarrr The above reminds me, we also need to review in which other places outside the dashboard we still have blue CTAs and see whether/how we want to replace them.

eugene-manuilov commented 2 years ago

There is a POC, which explores the structure and responsive layout and can be used as a reference/starting point.

@techanvil, please, avoid using PoC when it is not necessary. We strongly discourage using the "PR as IB" approach. Sometimes that's okay to do it, but usually, it should be either something urgent, super trivial, or something that's hard to figure out how to implement. This time it is just a task to create a few additional components which obviously doesn't need a PoC.

  • Create a default export function called useActivateModuleCallback which takes a moduleSlug argument.

We also need to add js tests for the new hook.

Make use of the Button component for the blue button. The styling will need a bit of a tweak for this component.

Let's better use the <Link onClick={...}> element instead of the Button to avoid a need to update styles. The Link component will use the button tag under the hook if neither href nor to props are passed.

  • Pass supportURL to the Button via the href prop and specify target="_blank".

Same here. Let's use the <Link href={ ... } external /> element instead of the Button.

techanvil commented 2 years ago

Thanks for taking a look at this, @eugene-manuilov.

There is a POC, which explores the structure and responsive layout and can be used as a reference/starting point.

@techanvil, please, avoid using PoC when it is not necessary. We strongly discourage using the "PR as IB" approach. Sometimes that's okay to do it, but usually, it should be either something urgent, super trivial, or something that's hard to figure out how to implement. This time it is just a task to create a few additional components which obviously doesn't need a PoC.

I appreciate the point you're making, but in this case, I did find it difficult to envisage how the responsive aspect might work, and felt it hard to write the IB with that conceptual gap in my knowledge. That's why I ended up doing some exploratory work myself, and I figured in this case it would be useful to share the work as a reference. I'll certainly endeavour not to overdo it with this approach, though.

  • Create a default export function called useActivateModuleCallback which takes a moduleSlug argument.

We also need to add js tests for the new hook.

Good shout. I've updated the IB accordingly.

Make use of the Button component for the blue button. The styling will need a bit of a tweak for this component.

Let's better use the <Link onClick={...}> element instead of the Button to avoid a need to update styles. The Link component will use the button tag under the hook if neither href nor to props are passed.

  • Pass supportURL to the Button via the href prop and specify target="_blank".

Same here. Let's use the <Link href={ ... } external /> element instead of the Button.

The Figma designs have been updated since the initial description was written:

image

As such the Button component is a closer match, stylistically, to the best of my knowledge. I did initially take a look at Link.

eugene-manuilov commented 2 years ago

Thanks, @techanvil. IB ✔️

techanvil commented 2 years ago

Hi @marrrmarrr, I have implemented these new CTAs, but have noticed that the Goals one in particular has some issues related to responsive layout - most notably at the width of 601px where the Search Funnel widget has just switched to the four column layout, also at a couple of other points where it is a bit squeezed.

Please could you take a look at the screen captures below and consider whether we want to revisit the design on this one a bit.

cc @felixarntz @aaemnnosttv

image

I've recorded a video of the Goals CTA in all its responsive glory:

https://user-images.githubusercontent.com/18395600/154449681-f0cfbd44-7bc6-4d62-a09e-310a602703d0.mp4

And also the Analytics one for good measure:

https://user-images.githubusercontent.com/18395600/154445938-5f9c1a06-ed09-431d-9497-412c9ec0c86c.mp4

techanvil commented 2 years ago

Re. the above, I have discussed this "offline" with @marrrmarrr and we are going to tweak the Search Funnel widget layout so it displays the 2x2 grid for the tablet viewport (601-960px) - but only when the new Goals CTA is displayed.

In other words, this layout for tablet:

image

But, still this when the CTA is not displayed:

image

mohitwp commented 2 years ago

QA Update : ❌

@techanvil I have a number of observations and realise they could be because we're still engineering and/or figma design.

  1. Font size and Font color issue (Main and entity dashboard) > Implemented font size for CTA content differs from Figma design. image image image

  2. Graph does not have "gathering data..." text and dates on x-axis as it does in Figma design. (Main and Entity Dashboard)

image

  1. If the user cancels the analytics set up, then Blue CTA box gets display. I'm wondering we are planning to change this 'Blue Box' CTA also or we keep it same as now ?

image

  1. My Laptop screen size is 1519 × 746 and Goal CTA is showing stretched in this size. image

  2. In Figma design search funnel have tool tip. But we have added no tool tip. image

  3. Search funnel gathering data then clicking on 'Total clicks', 'Total impressions' or Unique visitors from search throwing error.

https://user-images.githubusercontent.com/94359491/156732808-e8755627-545d-4dbf-8c9c-b39d788df1a2.mp4

Note : This issue does not cover moving the CTAs below the chart on mobile. That will be covered in https://github.com/google/site-kit-wp/issues/4695, a separate issue that depends on this one.

Admin bar CTA will get update by : https://github.com/google/site-kit-wp/issues/4867 WP Dashboard will get update by: https://github.com/google/site-kit-wp/issues/4868

techanvil commented 2 years ago

Hi @mohitwp, thanks for your observations here, it does appear though that they are mostly either non-issues, or out of scope here, as you have speculated.

As I've recently mentioned on another issue, the Figma mocks are just that, they are mocks as opposed to pixel perfect designs. The font sizes and dimensions shouldn't be taken as needing to be adhered to exactly, and it's more important that we maintain a consistent look and feel in the app. I have noticed the font sizes seen in Figma tend to be smaller than the actual implemented font size.

Also - please bear in mind this issue is only about implementing the new Setup Analytics and Create Goal CTAs.

With that in mind I'll address your points.

  1. Font size and Font color issue (Main and entity dashboard) > Implemented font size for CTA content differs from Figma design. image image image

The existing UI areas you've highlighted have not been affected by this ticket, are out of scope, and the fonts/colors are not expected to conform exactly to the Figma mocks. These are non-issues here.

The new areas, i.e. the CTAs, are certainly within scope, but as mentioned the fonts aren't supposed to be an exact match and should simply be in accordance with existing styles. Again, although in scope, I think these are OK, although it's certainly fair to query it.

  1. Graph does not have "gathering data..." text and dates on x-axis as it does in Figma design. (Main and Entity Dashboard)

image

That is outside the scope of this ticket, with the "gathering data" text being added via other issues.

  1. If the user cancels the analytics set up, then Blue CTA box gets display. I'm wondering we are planning to change this 'Blue Box' CTA also or we keep it same as now ? image

This is a good question! Pretty sure that should be changed too. Although, arguably this could be done as part of this ticket, I think we'd be better off creating a new ticket to cover it, as we don't have some necessary details about what this CTA should look like, if it needs new copy, etc. Would you mind creating a followup issue on this?

  1. My Laptop screen size is 1519 × 746 and Goal CTA is showing stretched in this size. image

I'm not sure what the answer here is. The Figma mockups don't go into detail about the responsive behaviour, and there was quite a lot of back-and-forth during code review about how the responsive aspect should work. I'm going to ask @marrrmarrr or @felixarntz to weigh in here, I'll post a separate comment about it. My thoughts are it should probably pass QA so a broader audience can assess it and we can subsequently make any tweaks as needed.

  1. In Figma design search funnel have tool tip. But we have added no tool tip. image

Again, this is out of scope, this issue only being related to the new CTAs.

  1. Search funnel gathering data then clicking on 'Total clicks', 'Total impressions' or Unique visitors from search throwing error.

    Recording.72.mp4

Again, this is out of scope, please feel free to raise a separate issue on this.

techanvil commented 2 years ago

Hi @marrrmarrr or @felixarntz, I'm copying this excerpt from the above comment to make it a bit easier to focus on.

Please could you advise on how to tackle this concern about the CTA as raised by @mohitwp during QA?

  1. My Laptop screen size is 1519 × 746 and Goal CTA is showing stretched in this size. image

I'm not sure what the answer here is. The Figma mockups don't go into detail about the responsive behaviour, and there was quite a lot of back-and-forth during code review about how the responsive aspect should work. I'm going to ask @marrrmarrr or @felixarntz to weigh in here, I'll post a separate comment about it. My thoughts are it should probably pass QA so a broader audience can assess it and we can subsequently make any tweaks as needed.

mohitwp commented 2 years ago

@techanvil thank you for taking the time to look at the points in my comment, appreciate it.

I will make a note of these and move this ticket on since it passes the QAB.

For Responsive issue, I will create a separate ticket as we need to look into different view ports. Issue is coming in small screens (laptops). In mobile devices, CTA is looking good. Issue with position of CTA in mobile will get resolve by https://github.com/google/site-kit-wp/issues/4695

cc @felixarntz @marrrmarrr

mohitwp commented 2 years ago

QA Update ✅

Verified:

Regarding issue mentioned here - https://github.com/google/site-kit-wp/issues/4694#issuecomment-1059381207 . I will create a separate ticket for this.

techanvil commented 2 years ago

Thanks @mohitwp! Please can you post a link to the new ticket here (or mention this ticket number #4694 within the new ticket, so as to create a link), for future reference?

mohitwp commented 2 years ago

@techanvil Here is the link : https://github.com/google/site-kit-wp/issues/4930