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

Create Sign in With Google banner notification to prompt user to enable feature #9335

Closed
2 tasks done
tofumatt opened this issue Sep 11, 2024 · 13 comments
Closed
2 tasks done
Labels
Module: Sign in with Google Sign in with Google (SiwG) related issues. P0 High priority QA: Eng Requires specialized QA by an engineer Team S Issues for Squad 1

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented Sep 11, 2024

Feature Description

A banner prompting users to enable Sign in with Google should be added to Site Kit. See the Figma design/screenshot below for the banner and its contents.

Image


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

Acceptance criteria

  • The banner should only appear when the signInWithGoogleModule feature flag is enabled.
  • A new notification banner should appear for users that have not connected the Sign in with Google module.
  • It should follow the Figma design
  • The banner should only appear when:
    • The user can enable Sign in with Google (eg. their website supports HTTPS)
    • They have not permanently dismissed the banner
  • The "Set up Sign in with Google" button should direct the user to the Sign in with Google setup flow, the same as connecting the Sign in with Google module from the "Connect more Services" modules page
  • The "Maybe later" button should dismiss the notification permanently.

Implementation Brief

  • Add assets/js/components/notifications/SignInWithGoogleModuleSetupCTAWidget.js
    • You can check GatheringDataNotification component for an example of using this new Notifications infrastructure
    • Include 2 new props - id and Notification
    • Wrap the component with Notification component passed as the prop
    • Create a new layout - assets/js/components/notifications/NotificationWithSVG.js
      • It should render SVG next to the content on desktop and bellow the CTA's on mobile and tablet, following the Figma designs
      • Extract the layout from BannerNotification,you can re-using the logic around WinImageSVG
      • You can check assets/js/googlesitekit/notifications/components/layout/NotificationWithSmallSVG.js for example. It should contain only the needed layout including the passed SVG, title, description and actions
    • Set up Sign in with Google CTA callback should redirect to the module setup page
    • Use useActivateModuleCallback( 'sign-in-with-google' ) as the callback
    • Maybe later CTA should dismiss notification, ActionsCTALinkDismiss component should be used for both CTA's, and for dismiss it is enough to provide dismissLabel prop. dismissExpires is 0 by default, which is correct value for this case, where we are dismissing it permanently.
  • Update assets/js/googlesitekit/notifications/register-defaults.js
    • Register the notification, following the existing patterns already added
    • For checkRequirements:
      • If signInWithGoogleModule feature flag is not enabled return early
      • Check if site is using HTTPS, using isURLUsingHTTPS helper function, and pass it a home URL, obtained from getHomeURL selector from CORE_SITE datastore
      • Check if sign-in-with-google module is connected using isModuleConnected from CORE_MODULES datastore, and return early if it is not connected
    • For priority, bump it by 10 from the last added banner notification (not including error ones, which start from 150, the regular ones start from 300).
    • Use NOTIFICATION_AREAS.BANNERS_BELOW_NAV for areaSlug
    • Include isDismissible property with true for value

Test Coverage

  • Add stories file and VRT for the banner

QA Brief

  • Enable the feature flag for SiWG and test this feature on websites that are secure, i.e. HTTPS only. Also ensure that SiWG is not connected or setup in the Site Kit Settings tab. Ensure the Gathering data or Zero Data or any other Error Notification banners are dismissed.
  • Now go to the dashboard and the Setup CTA Banner will appear. This does take a couple of seconds to load currently and is expected. The load time will be improved further in Improve rendering time of newly refactored "Setup Success" subtle notifications #9488 and Improve rendering time of newly added "Setup CTA" SiWG notification #9568.
  • Test the ACs along with other generic notification behaviour such as the GA events for confirm_notification, dismiss_notification, view_notification and click_learn_more_link.
  • The notification should not reappear if the module is already set up and connected, or if the notification was dismissed or if the site does not use HTTPS.

Changelog entry

  • Add new feature notification for Sign in with Google module.
@tofumatt tofumatt added P1 Medium priority Module: Sign in with Google Sign in with Google (SiwG) related issues. labels Sep 11, 2024
@tofumatt tofumatt self-assigned this Sep 11, 2024
@binnieshah binnieshah added the Team S Issues for Squad 1 label Sep 20, 2024
@binnieshah binnieshah added the Next Up Issues to prioritize for definition label Oct 15, 2024
@tofumatt tofumatt removed their assignment Oct 17, 2024
@zutigrm zutigrm self-assigned this Oct 18, 2024
@eugene-manuilov
Copy link
Collaborator

For checkRequirements:

  • If signInWithGoogleModule feature flag is not enabled return early
  • Check if site is using HTTPS, using isURLUsingHTTPS helper function, and pass it a home URL, obtained from getHomeURL selector from CORE_SITE datastore

@zutigrm, the following requirement is missing:

A new notification banner should appear for users that have not connected the Sign in with Google module.

@zutigrm
Copy link
Collaborator

zutigrm commented Oct 18, 2024

@eugene-manuilov ah indeed! Thanks, IB updated

@eugene-manuilov
Copy link
Collaborator

  • Check if clientID setting from Sign in with google module is empty, to return early

@zutigrm, we should use isModuleConnected to check whether or not the module is connected.

@zutigrm
Copy link
Collaborator

zutigrm commented Oct 18, 2024

@eugene-manuilov Updated

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Oct 18, 2024
@eugene-manuilov
Copy link
Collaborator

Thanks, IB ✔

@eugene-manuilov eugene-manuilov removed their assignment Oct 19, 2024
@jimmymadon jimmymadon self-assigned this Oct 21, 2024
@eclarke1 eclarke1 added P0 High priority and removed P1 Medium priority labels Oct 21, 2024
@jimmymadon jimmymadon removed their assignment Oct 29, 2024
@zutigrm zutigrm self-assigned this Oct 29, 2024
@tofumatt tofumatt removed their assignment Nov 5, 2024
@kelvinballoo kelvinballoo self-assigned this Nov 6, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠

Hi @jimmymadon , I've set up a site but the banner isn't showing at all.
Video is attached for reference.

  • Feature flag is on, develop branch is on, the module SiWG is disconnected, site is HTTPS
  • Am I missing anything else?
https://github.com/user-attachments/assets/bff972e0-b214-4fe3-bf8b-e8805b6a775b

@jimmymadon
Copy link
Collaborator

@kelvinballoo I should've just seen the video first! You've got the Zero Data Notification banner showing! So in this case, if you dismiss or action that banner, the Sign in with Google banner should then appear. This behaviour will change after #9568 is merged though as the Notification Banners and Setup CTA banners will have their own "queue"!

@jimmymadon jimmymadon removed their assignment Nov 6, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Nov 7, 2024

QA update ⚠

Thanks @jimmymadon . The banner showed up after dismissing it the Zero data banner.
I had a first round of testing and I have some observations/queries below:

ITEM 1:
The 'Set up Sign in with Google' button text should have been 'Google Sans Display' based on Figma.
Currently, it's 'Google Sans Text'.
If this is standardise with other CTAs of the plugin, that's fine and this can be ignored.

Figma:

Image

Actual:

Image


ITEM 2:
When viewed on mobile, the image layout is smaller than what is on Figma on the same breakpoint width.
The 'Maybe later' button is also sitting below the 'Set up Sign in with Google' button currently. On Figma, it's next to each other.
Photos attached for reference.

Figma:

Image

Actual:

Image


ITEM 3:
Similar observation on tablet. The graphic is smaller compared to figma on the same breakpoint width.
Photos attached for reference.

Figma:

Image

Actual:

Image


ITEM 4:
On desktop Safari, the graphic is not showing up at all.
Tested on Safari Version 17.6 (19618.3.11.11.5), MacOS Sonoma 14.6.1
It appears for Edge, Firefox and Chrome.

Image

Also tested on Browserstack:

Image


ITEM 5:
This is a query around the AC:

The notification should not reappear if the module is already set up and connected, or if the notification was dismissed 
 or if the site does not use HTTPS.
  • If let's say I have connected the module with incomplete setup and proceeded to disconnect it. I never dismissed the banner. Should the banner re-appear in that case?
  • Currently all the sites that I would create on tastewp and instawp would have https. Is there an easy way to test for HTTP website?

@jimmymadon
Copy link
Collaborator

jimmymadon commented Nov 13, 2024

@kelvinballoo

ITEM 1

Yes, that is correct. I've created Issue #9675 that should fix this.

ITEM 2 and ITEM 3 SVG Images

Again, #9675 should improve the way we render SVGs on different viewports. However, I have fixed the Maybe later button stacking instead of being side by side in my follow up PR which is now merged.

ITEM 4

Fixed in the follow up PR.

ITEM 5
If let's say I have connected the module with incomplete setup and proceeded to disconnect it. I never dismissed the banner. Should the banner re-appear in that case?

Yes, I believe so. The conditions in the AC are all mutually independent of each other. So in the case you mention, the banner was never dismissed and the module is fully disconnected.

Is there an easy way to test for HTTP website?

I think this can be done locally by any engineer as QA:Eng if everything else is tested. This might be simpler than you trying to force a HTTPS website to work with HTTP.

@jimmymadon jimmymadon removed their assignment Nov 13, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠

Thanks @jimmymadon .
Noted that items 1-3 will be fixed under #9675. ✅


ITEM 2
Tested for the button placement and it's displaying as expected with the buttons next to each other and not stacked.

Image


ITEM 4
Verified on Safari and the graphics are now displaying as expected.

Image


ITEM 5
RE: Banner re-appearance
I have a particular scenario:

  • I see the banner

  • I click 'Set up SiWG' on the banner

  • I do not complete the setup

  • I disconnect the SiWG module

  • I go to the homepage. Should the 'SiWG' banner appear again? I am assuming no because currently it doesn't and I believe it's because we already clicked on the banner. That said, if the expectation is for the banner to re-appear, that would need to be further looked into in another issue.
    Video is attached for reference

    9335.-.special.scenario.mov

RE: HTTP
Noted, I will add the QA:Eng label for an engineer to test locally that an HTTP site should not have the banner to show.

Assigning ticket to @jimmymadon to review ITEM 5.

@kelvinballoo kelvinballoo added the QA: Eng Requires specialized QA by an engineer label Nov 13, 2024
@jimmymadon
Copy link
Collaborator

I go to the homepage. Should the 'SiWG' banner appear again? I am assuming no because currently it doesn't and I believe it's because we already clicked on the banner. That said, if the expectation is for the banner to re-appear, that would need to be further looked into in another issue.

@tofumatt What do you think the behaviour should be here? When we click on the CTA or the Maybe later button, the notification is "dismissed" purposefully. This is fine until someone disconnects the module. Should we be removing the dismissal when we "disconnect" the module? I think we do this for some other modules. If yes, then we should create a separate issue as that will involve adding a simple action on when the module is disconnected.

@kelvinballoo kelvinballoo removed their assignment Nov 13, 2024
@tofumatt
Copy link
Collaborator Author

I don't think we should show the CTA again after a user has purposefully connected and disconnected the module. Presumably they did so for a reason, so reminding them to connect something they've already disconnected quite intentionally seems bad UX. I think it's fine that the banner doesn't appear again 👍🏻

@tofumatt
Copy link
Collaborator Author

QA ✅ from me on the QA: Eng section, so moving to Approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Sign in with Google Sign in with Google (SiwG) related issues. P0 High priority QA: Eng Requires specialized QA by an engineer Team S Issues for Squad 1
Projects
None yet
Development

No branches or pull requests

8 participants