Add the Introductory Popup / Banner (Storybook) about site-kit-wp HOT 8 CLOSED

@techanvil Thank you for the response. I made a mistake in linking the slack message. This is the right one. Oops. cc @kelvinballoo

from site-kit-wp.

QA Update ✅

Thanks @techanvil , @kuasha420 .

Noted on all the points clarified.

Tested the outstanding items as follows:

• ITEM 1: ✅
  The popup appears on the opposite side of the WP sidebar and for a language with opposite writing direction, it doesn't go under the WP Sidebar.

  

• ITEM 2: ✅
  The position of the pop up on desktop is correct now.
  It's 42px from the right side.
  It's 22px from the bottom side.

  

• ITEM 6: ✅
  There is no longer the white gap below the image.

  

These are looking good.
Moving ticket to Approval.

from site-kit-wp.

IB ✅

from site-kit-wp.

QA Update ❌

Tested on WP 6.5.2 and PHP 8.0 on develop branch.
I've detailed the issues/clarifications required below.

ITEM 1:
The QAB states: "It should be placed on the side apposite to the WP Admin Sidebar and not go under the sidebar."
What do we mean by not going under the sidebar?

ITEM 2:
The position of the pop up on desktop isn't correct.
The implementation shows equal margin on the right side and bottom side. But according to figma, the distance is bigger on the right hand side.

Please help to double check the same for tablet if required.

Notice that on figma, the right side is longer. Unless, the bottom on the figma is actually not the screen bottom:

Either way, we still need to work on the position because when I enable the audiencesegmentation flag, you will notice that the popup aligns very closely to the back element (which is not the case on figma):

ITEM 3:
Question: Do we use the font 'Google Sans Display' and 'Google Sans text' be used interchangeably?
The reason I ask is because sometimes I don't see them aligned:

• For the CTA buttons on desktop figma, it's supposed to be 'Google Sans Display'.

  Implementation is currently 'Google Sans Text'

  Figma:

  Implementation:
  

• On mobile the Title implemented is 'Google Sans display' but in figma it's 'Google sans Text'.

  Implementation:

  Figma:
  

ITEM 4:
On figma, the CTA text margin on top and bottom is '10px'
Current implementation is 8px.

ITEM 5:
The following actually do not warrant a fix because they are very minor and might not even be noticeable but I am raising them anyway in case we want to be pixel perfect with the designs.

The green colour used is supposed to be B8E5CA instead of B8E6CA.

On figma, the following margins are 25px and current implementation is 24px.
While I think 24px should be the norm, I'm just raising it because there is a deviation from figma.

ITEM 6:
The image on mobile is 267px in width currently.
Based on figma, it should be 295px.
2nd thing to note is that on figma, the image stretches to the bottom of the card and there is no white line below it.

Implementation:

Figma:

ITEM 7:
These are all margin/padding issues on mobile.
The following distances are all 20px on figma but on implementation, it's 16px:

Figma:

Implementation:

The distances on the sides should be 16px based on figma but it's currently larger than that.

Figma:

Implementation:

There should be more space below the buttons and the image (27px).
Implementation is currently 16px.

Figma:

Implementation:

from site-kit-wp.

Thanks @kelvinballoo. I'll try to address your observations. I'll address whatever needs addressing in a follow up PR.

1. Sorry for not being somewhat vague here. Please see this slack message for additional context.
2. Nice find. Looks like the margin is 32px on both right and bottom on tablet, but 42px right, 22px bottom on desktop. I'll spin up a PR to update this.
3. Can not exactly answer your question about interchangeably using the font, but the examples you've listed are consistent with rest of Site Kit, the CTA is a reusable button used everywhere, and the h3 tag also have same font applied throughout the plugin. I don't think we need to update this.
4. This is the same as above, the CTA button is a reusable component and consistent across the plugin.
5. Interesting observations! The B8E6CA is coming straight from the exported SVG from Figma. So I'm not sure why the svg and the UI is different. I've exported the file again, and the color comes out as B8E6CA. Which is the case for all the overlay images. This is fine. The margin difference is fine as well as we're using standard SK gap values.
6. That is valid, I'll fix it up in follow up PR.
7. Interesting observations!
   a. For padding around the widget, I've matched it with other CTA/Banner/Widget margin/padding on mobile for consistent look with our defined gaps. I think they should remain 16px but will ask in the channel to confirm.
   b. For margins on the side, can you check if the margins matches with other widgets and Banners on SK Dashboard? I remember matching that with the rest of the widget/CTA.
   c. I've intentionally kept it 16px, following our gaps elsewhere in the plugin. It should either remain 16 or be 24 px, 27 seems like an odd choice. I'll ask about this one in slack as well and confirm.

Thank you!

from site-kit-wp.

Hey @kuasha420, reviewing the points in your reply above:

1. Sorry for not being somewhat vague here. Please see this slack message for additional context.

Tbh, I am actually a bit confused here - I'm not clear on what the additional context should be taken from the linked thread.

Furthermore though, in theory we shouldn't be integrating this popup/banner into Site Kit as part of this issue. The component is supposed to be shown conditionally upon setting up the Audience Segmentation feature, just adding it to Storybook in this issue and then integrating it via #8172.

Still, I suppose it's behind the feature flag and so it doesn't really hurt to show it unconditionally for now so we can verify it works as expected when integrated, and we can simply apply the relevant conditions as part of 8172.

Anyhow, @kuasha420, please can you double check the linked thread for relevancy and add a bit of further clarification for this point?

...
6.

Your replies to points 2 through 6 LGTM.

7. Interesting observations!
   a. For padding around the widget, I've matched it with other CTA/Banner/Widget margin/padding on mobile for consistent look with our defined gaps. I think they should remain 16px but will ask in the channel to confirm.
   b. For margins on the side, can you check if the margins matches with other widgets and Banners on SK Dashboard? I remember matching that with the rest of the widget/CTA.
   c. I've intentionally kept it 16px, following our gaps elsewhere in the plugin. It should either remain 16 or be 24 px, 27 seems like an odd choice. I'll ask about this one in slack as well and confirm.

As you've pointed out, we should be aiming for consistency within the plugin. Sometimes Figma is a bit out of alignment with what's already there - it's something we should certainly aim to improve going forward but in the meantime we do have to use our judgement to assess what the concrete implementation should look like.

In this case, it's clear the margins are consistent with other similar banners e.g. the AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification component which can be seen here in Storybook, as well as with the margins surrounding other widgets which use the standard 16/24px gap size. I.e., the widgets should be aligned as illustrated here, and the top margin should use the same value:

So, to summarise point 7 I would say the implementation LGTM as it stands.

from site-kit-wp.

@techanvil @kelvinballoo The first point of ITEM 6 regarding the size is also fine I think, it maintains the aspect ratio and and changes size based on screen size on mobile, so it will not be exactly same as Figma and most other SVGs in the plugin behaves that way.

With that said, the follow up PR that fixes the overlay position on tablet/desktop (bottom/right margin) and the whitespace under the SVG on mobile is now in CR.

Cheers

from site-kit-wp.

Thanks @kuasha420. The followup PR is merged, back to you for another look, @kelvinballoo.

from site-kit-wp.