site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Add CTA for users with Incomplete User Input answers in Settings page

Open kuasha420 opened this issue 3 years ago • 2 comments

Feature Description

In the current Admin Settings page, there is a CTA/Banner displayed when user has not completed the User Input Survey, it should be updated or refactored to use the new Figma design.


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

Acceptance criteria

  • A CTA should be shown when an user who has not taken the User Input questions visits Admin Settings page.
  • The CTA should follow the figma design.
  • Clicking the CTA will take them to User Input page.
  • The section should only be shown when both Search Console and Analytics are connected and not in gathering data state.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

kuasha420 avatar Sep 25 '22 21:09 kuasha420

  • The section should only be shown when both Search Console and Analytics are connected and not in gathering data state.

My understanding is that the gathering state would only affect the presence of the CTA on the dashboard; settings should be independent of this, especially since this requirement is more about the key metrics widget than user input. We should discuss this with @marrrmarrr but I don't think the gathering state should apply here.

aaemnnosttv avatar Oct 12 '22 20:10 aaemnnosttv

@kuasha420 let's remove this condition about the gathering state for now and we can check with @marrrmarrr when she gets back. It would be a rather small change to make later I think but my guess is we probably won't need it.

One thing I'm not entirely clear on is which issue we're adding the new settings container/area in. Can you clarify that part?

aaemnnosttv avatar Oct 14 '22 20:10 aaemnnosttv

@aaemnnosttv There was a discussion about it in Figma. Feel free to have a look there. If we can skip that requirement for now, I have no issues with it. Cheers.

kuasha420 avatar Oct 20 '22 13:10 kuasha420

  • Check if Analytics and Search Console are connected using the isModuleConnected selector.

@hussain-t this is not sufficient because we also need to make sure that these modules are not in the gathering state as AC says: and not in gathering data state. Also, let's explicitly mention that the CTA shouldn't render if this condition is not met.

  • ctaComponent should have the value of a <Link> component with the href and onClick props with the relevant values, which are the same as the ctaLink and ctaLabel props.

Do we need this only to change styles of the link? If so, I think we can do it using css, so there won't be any need to update these properties.

eugene-manuilov avatar Oct 26 '22 19:10 eugene-manuilov

@eugene-manuilov, I didn't include checking for gathering data state due to this comment. I have no issues adding that condition.

Do we need this only to change styles of the link? If so, I think we can do it using css, so there won't be any need to update these properties.

IMO, composing it by passing the <Link> component to the ctaComponent prop will be a cleaner approach. That will have full control of whatever we are passing rather than creating new styles. LMK WDYT?

hussain-t avatar Oct 27 '22 05:10 hussain-t

@kuasha420, based on the Figma discussion, I think we need to check for both the gathering data state and zero data state, right? The AC mentions checking only for the gathering data state.

hussain-t avatar Oct 27 '22 05:10 hussain-t

@hussain-t I remember just gathering data would suffice, but I can't find the discussion right now. Maybe it was in one of the resolved comments in the Design doc.

I think @felixarntz or @marrrmarrr is better suited to answer this anyway, since I can't find the original discussion.

Cheers.

kuasha420 avatar Oct 27 '22 06:10 kuasha420

Thanks, @kuasha420; I removed the condition to check for a zero data state from the IB. We can add it back if necessary based on @felixarntz or @marrrmarrr answer.

hussain-t avatar Oct 27 '22 06:10 hussain-t

Ok, thanks, @hussain-t. IB ✔️ for me. Let's have the gathering state information for now. If we get a confirmation that we don't need it, we will adjust IB later.

eugene-manuilov avatar Oct 27 '22 12:10 eugene-manuilov

QA Update ⚠️

@derweili

  • Verified on dev.
  • CTA not appears when Analytics is disconnected.
  • CTA not appears when analytics or search console is in gathering state.
  • Verified CTA appears on Site Kit Dashboard and under Admins settings.

Q-1 ) I've not found exact design of CTA in Figma design. I compared this with Figma link mentioned in AC. CTA title font size is different in this case. Do we have CTA figma design ?

Q-2) If UA is connected but GA4 is not connected. Are we going to show CTA in this case ? Currently, we are showing.

image

Q-3) Graphics under CTA looks small and not proper aligned on desktop and mobile devices.

image

image

Q-4) If analytics is not connected then in this case we are not showing CTA under Admin settings and banner on WP dashboard. Are we not going to allow user to give survey in this case ? I've noticed below scenario where user is able to give survey if analytics is not connected.

  1. Enable User input feature flag.
  2. Connect Search console and analytics.
  3. Go to Site kit dashboard. You will see a user input banner.
  4. Now open settings in another tab.
  5. Disconnect analytics.
  6. Do not refresh Site kit dashboard tab.
  7. Click on 'Personalize your matrices' under banner.
  8. User is able to give answers to all questions and survey is getting save.

mohitwp avatar Dec 12 '22 14:12 mohitwp

QA Update: ⚠️

@mohitwp @derweili I just wanted to add an observation that I have found. @nfmohit suggests that this issue comes from this ticket so reporting it here.

If Google Analytics is not connected, and I submit the user input questions, when I am redirected to the Site Kit Dashboard, an error appears in the console. Is this expected?

Google Site Kit API Error method:GET datapoint:report type:modules identifier:analytics error:"Module must be active to request data."

image

wpdarren avatar Dec 13 '22 08:12 wpdarren

@wpdarren How you are able to submit the user input questions ? Because if analytics is not connected then as per changes introduced in this ticket we are hiding Banner on WP dashboard and CTA under Admin settings ? Although, this is possible through the steps I mentioned above in comment. Let me know if you followed any other steps ?

Q-4) If analytics is not connected then in this case we are not showing CTA under Admin settings and banner on WP dashboard. Are we not going to allow user to give survey in this case ? I've noticed below scenario where user is able to give survey if analytics is not connected

  1. Enable User input feature flag.
  2. Connect Search console and analytics.
  3. Go to Site kit dashboard. You will see a user input banner.
  4. Now open settings in another tab.
  5. Disconnect analytics.
  6. Do not refresh Site kit dashboard tab.
  7. Click on 'Personalize your matrices' under banner.
  8. User is able to give answers to all questions and survey is getting save.

@derweili if user already answered all questions under user input survey and site goal is set. After that if user disconnect analytics then on Site kit dashboard I'm getting the same error in console which @wpdarren reported.

image

mohitwp avatar Dec 13 '22 09:12 mohitwp

@mohitwp Search console is in gathering state but Analytics is not connected. In this scenario I do not get the Customize Site Kit to match your goals on dashboard. So, I am able to answer the questions and submit them. I then receive the banner confirming that the questions have been successfully answered, and this is where the error I mentioned appears.

The section should only be shown when both Search Console and Analytics are connected and not in gathering data state.

So, the scenario I mentioned above, it does make sense to not get the Customize Site Kit to match your goals banner on dashboard according to the AC. Happy to jump on a huddle if it helps to clear this up.

wpdarren avatar Dec 13 '22 14:12 wpdarren

The error comes from here: https://github.com/google/site-kit-wp/blob/610f6fd2ce5238eb4024d27f0335fc0e1311c381/assets/js/components/notifications/UserInputSettings.js#L67-L69

isGatheringData fetches a report from the API which then returns this error.

So I think, inside isGatheringData we should verify if Analytics is connected. If not, the api should not be requested and isGatheringData should return false.

@mohitwp do you agree?

derweili avatar Dec 13 '22 15:12 derweili

@mohitwp Regarding you first questions about the UI and figma. I actually don't know. As I did not change the UI with this issue/pr. The component existed before and I just changed the conditional display logic. Therefore I did not need figma designs.

derweili avatar Dec 13 '22 16:12 derweili

QA Update ❌

@derweili Thank you ! Please look into below observations.

Issue > Under CTA banner description, it references 5 questions, but we have only 3 questions currently.

image

image

image

Issue 2> When analytics or search console is in zero data state. Then on Site Kit Dashboard I can notice flicker. Initially it shows User Input CTA banner then after few seconds zero date state banner gets display.

https://user-images.githubusercontent.com/94359491/207424530-7eba4fd2-5557-495c-9aeb-b39e28d816e2.mp4

@aaemnnosttv @kuasha420 Can you please provide your feedback on listed questions ?

Q-1) AC and QAB says CTA banner should match Figma design. CTA design following the layout mentioned for admin settings page under figma design (in the AC's). But, I need actual CTA banner Figma design to do comparison. Do we have actual CTA Figma design ?

Q-2) If Google Analytics is not connected, and I submit the user input questions, when I am redirected to the Site Kit Dashboard, an error appears in the console. Is this expected?

Google Site Kit API Error method:GET datapoint:report type:modules identifier:analytics error:"Module must be active to request data."

image

@derweili have question regarding it which you can answer better.

Q-3) If UA is connected, but GA4 is not connected. Then in this case we are showing Incomplete User Input CTA banner. Is this expected ?

image

mohitwp avatar Dec 13 '22 19:12 mohitwp

@mohitwp

Issue > Under CTA banner description, it references 5 https://github.com/google/site-kit-wp/issues/5895#issuecomment-1348873878s, but we have only 3 questions currently.

Probably, the user input questions changed from 5 to 3 question but this notification wasn't updated. So we should change this.

Issue 2> When analytics or search console is in zero data state. Then on Site Kit Dashboard I can notice flicker. Initially it shows User Input CTA banner then after few seconds zero date state banner gets display.

I suspect this comes from the issue described here #5933, which is also set as a blocker for this issue. The gathering data state is not available during the initial render but calculated on runtime. So after the state is calculated, a re-render happens which results in this flicker. So i think we should wait for #5933 and verify if the flicker persists.

derweili avatar Dec 14 '22 11:12 derweili

@mohitwp I'm a bit confused about your CTA question. This issue is only about the CTA in Settings page. This is the CTA

Screenshot_20221214_173241

The Dashboard CTA will be updated as part of an issue in Key Metrics Epic.

I'm not sure where the console error is coming from.

kuasha420 avatar Dec 14 '22 11:12 kuasha420

Thank you, @kuasha420, and sorry for the confusion. I got confused due to same CTA under settings and on dashboard. Currently, Both are same and have same logic implemented for visibility.

@derweili As per @kuasha420 comment above can you please update the CTA content and font size as per figma design

Figma -

image

Current CTA -

image

mohitwp avatar Dec 14 '22 12:12 mohitwp

QA Update ✅

  • Verified on main.
  • If analytics is not connected then CTA under settings is not showing.
  • If analytics or search console is in gathering state then CTA unde setting is not showing.
  • CTA title font size in Figma is 18px but as per Arafat we follow SCSS that's why we implemented font size 20px.
  • Font color for description in Figma is #161B18, but for consistency with current style implemented font color is #131418.
  • Note : While doing setup user input flow - user don't have option currently to opt out from questions and also when we try to do analytics set up by accessing settings then also user get navigate to user input flow. So, to test scenario where user do not submit answers to user input questions. I enabled the feature flag after completing site kit setup.

image

mohitwp avatar Dec 15 '22 11:12 mohitwp

Approval

⚠️ There are some details about the implementation that are not quite as designed.

For example, because this is implemented as a banner notification, it has an extra 10px of horizontal padding which puts the content out of alignment with other elements on the page. Also the design does not have this extra padding, it should only be 24px on desktop.

image

The design shows these consistently

image

This raises another related issue although not specific to this one which is a general discrepancy between the design and what we have in the plugin for everything else. I've kickstarted a conversation about this internally but it seems that the design is using GM3 styles rather than the GM2+ we have in the plugin right now. If that is the case then we shouldn't be trying to match the design exactly, but rather the elements/tokens of the design only.

This need not block the release as user input is still under active development, but we will need a new issue to adjust the styling/implementation here to fit in with the existing elements of the plugin page as designed, even if the details of the design do not match the spec.

aaemnnosttv avatar Dec 15 '22 21:12 aaemnnosttv

@aaemnnosttv I spent a little time to understand what we missed in QA, so set this up on a test site. Right now though the banner isn't even appearing in the admin settings. This is with Analytics and Search console connected but no questions answered. As you can see from the screencast, it flashes for a second then disappears.

I am not sure if another regression is at play here.

As you've said its not an issue for the release since it's behind a feature flag, but should we raise this issue separately?

@mohitwp are you able to see the same issue?

https://user-images.githubusercontent.com/73545194/208058511-a9f0522e-872d-4ab1-a277-cc94471d2101.mp4

wpdarren avatar Dec 16 '22 08:12 wpdarren

@wpdarren I verified it on Windows - Chrome, Mac - Safari, Chrome using BrowserStack. Banner is appearing for me. Analytics and search console connected and in data state -zero but no questions answered.

https://user-images.githubusercontent.com/94359491/208069716-e72bd4af-0681-4a9c-b1bd-20ea197e6928.mp4

mohitwp avatar Dec 16 '22 09:12 mohitwp

Right now though the banner isn't even appearing in the admin settings. This is with Analytics and Search console connected but no questions answered. As you can see from the screencast, it flashes for a second then disappears.

@wpdarren It seems @tofumatt was also able to replicate this here. I think it might be worth creating a new issue to investigate or re-open this one. What do you reckon?

CC: @aaemnnosttv @derweili @mohitwp

nfmohit avatar Dec 22 '22 06:12 nfmohit

@nfmohit thank you, I will create a ticket for this today so we can investigate.. 👍

wpdarren avatar Dec 22 '22 09:12 wpdarren

Thanks @nfmohit – this should indeed get a new issue for it as this one has already been completed for 1.90.

aaemnnosttv avatar Dec 22 '22 15:12 aaemnnosttv

Thanks @nfmohit – this should indeed get a new issue for it as this one has already been completed for 1.90.

Understood, thank you @aaemnnosttv!

nfmohit avatar Dec 22 '22 16:12 nfmohit

@nfmohit @aaemnnosttv just adding a comment here that the ticket has been created with regard to the banner not appearing in the admin settings when the user has not completed the questions.

wpdarren avatar Jan 04 '23 14:01 wpdarren