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 291 forks source link

Create setup page/flow for Sign in With Google #9336

Closed tofumatt closed 1 week ago

tofumatt commented 1 month ago

Feature Description

The setup page for Sign in with Google should be created for when a user activates Sign in with Google.

It should be only visible when the signInWithGoogleModule feature flag is enabled.

Screenshot 2024-10-05 at 12 52 23


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

eugene-manuilov commented 1 month ago

@tofumatt, this should be ready for review. Do you mind checking it? The only question that i have right now is which URL to use for the Learn More link?

tofumatt commented 1 month ago

@eugene-manuilov IB looks good to meβ€”we don't have a URL for that yet, as it will need to be created on the Site Kit help/documentation pages.

I'm having trouble finding the document we have with all of those URLs, but for now it should just be a new URL we use to point users to that explains the Henhouse process and how Sign in with Google works πŸ€”

Can we create a new help URL on the service that just doesn't point to anywhere yet while we create that new help page? πŸ€”

eugene-manuilov commented 1 month ago

Can we create a new help URL on the service that just doesn't point to anywhere yet while we create that new help page?

We can probably do that but we need to coordinate it with @jamesozzie or @adamdunnage. For now, I added a point to IB to use # as the learn more URL. I think we can update it later when we know exactly what should be there.

tofumatt commented 1 month ago

IB βœ…

jamesozzie commented 1 month ago

@tofumatt @eugene-manuilov I added a mention of that in our documentation task for this epic. Good shout.

mohitwp commented 1 week ago

QA Update ❌

Tested on dev environment.

@zutigrm

Issue 1 : Title font size in Figma is 20px and Font weight is 500 where as under actual implementation font size is 22px and weight is 400.

![Image](https://github.com/user-attachments/assets/fd23fb51-948b-4e0c-a11d-6060ea56b07e) ![Image](https://github.com/user-attachments/assets/e546dc5f-ad6a-4837-ba81-4640917ed6ad)

Issue 2: 'Get your client ID' CTA font weight is 500 in Figma where as under actual implementation it is 400.

![Image](https://github.com/user-attachments/assets/afb7b565-f758-4357-894d-35b7f7f19e9d) ![Image](https://github.com/user-attachments/assets/66846817-6d97-4336-bf30-f2cfb370146c)

Issue 3 : Module is getting auto connect even if user clicks 'Cancel' or just refreshes the page.

https://github.com/user-attachments/assets/327f9b96-9328-49fd-8005-fbfa6eebc482

Issue 4: In the Figma design, the "No ID entered" version includes an example client ID as a placeholder for the client ID field. However, in the actual implementation, we are not displaying any placeholder.

![Image](https://github.com/user-attachments/assets/48e3601f-e6ed-44e1-b799-a571f94ab53e) ![Image](https://github.com/user-attachments/assets/dc025dff-98ed-4527-b0b1-9d5b1e743c10)

Issue 5 : The Client ID currently lacks validation, which can be complex due to its structure. However, we should implement basic validation. As it stands, the Complete CTA gets enabled and user can complete setup if the user only enters spaces or any random special characters in the field.

![Image](https://github.com/user-attachments/assets/176f606d-616e-469c-b8d9-5dea3f1a803f)
zutigrm commented 1 week ago

@mohitwp Thanks

Issue 1 : Title font size in Figma is 20px and Font weight is 500 where as under actual implementation font size is 22px and weight is 400.

I will follow up on this with Sigal, as this title is consistent/same for all modules. If this is going to be new size, I will file a follow up issue to update this for all modules.

Issue 2: 'Get your client ID' CTA font weight is 500 in Figma where as under actual implementation it is 400. Issue 3: Module is getting auto connect even if user clicks 'Cancel' or just refreshes the page. Issue 5 : The Client ID currently lacks validation, which can be complex due to its structure. However, we should implement basic validation. As it stands, the Complete CTA gets enabled and user can complete setup if the user only enters spaces or any random special characters in the field.

Fixed

Issue 4: In the Figma design, the "No ID entered" version includes an example client ID as a placeholder for the client ID field. However, in the actual implementation, we are not displaying any placeholder.

This is mentioned in QAB, the field we use from 3rd party lib does not support that kind of layout with both label and placeholder, so it is consistent with all other fileds we use on module setup. There is also my comment on FIgma under that field, to mention that implementation is consistent with other fields

mohitwp commented 1 week ago

QA Update βœ…

Issue 2 : Fix

![Image](https://github.com/user-attachments/assets/718ed3a6-6733-47a1-8c80-55ed8a2cfe30)

Issue 3 : Fix

https://github.com/user-attachments/assets/0d72b304-b2cb-433d-8f49-d40fb788e0e4

Issue 5 : Fix

https://github.com/user-attachments/assets/d1452bc4-19c4-4787-986f-f95198cf601b
mohitwp commented 1 week ago

QA Update ❌

@zutigrm cc @wpdarren

Moving this back to execution. I missed this point before but as per AC - The module should only be allowed to be set up/activated on a site with HTTPS enabled. But, currently I'm able to activate and setup Sign in with Google on HTTP Site.

![Image](https://github.com/user-attachments/assets/50ac2f7c-b635-4904-8721-e3fafe50f9bf) ![Image](https://github.com/user-attachments/assets/808846d5-0e48-4199-9225-e17b72a068a0)
zutigrm commented 1 week ago

@mohitwp Thanks for your observation. Hm this issue was about adding a setup page, the https detection (or similar checks like ad blocker, etc) is usually done on the module connection settings screen. Let me check with @tofumatt what is planed here, if there is an issue for that separately, or we should include it as part of this one?

tofumatt commented 1 week ago

(This was answered on the stand-up call, but adding here for posterity.)

We should display a notice in the same way as the AdSense/RRM modules do when accessing these setup pages when the criteria for setup isn't met. It should look like: https://google.github.io/site-kit-wp/storybook/develop/?path=/story/modules-ads-setup-setupform--ad-blocker, with a message explaining that HTTPS is required to set up Sign in with Google.

wpdarren commented 1 week ago

QA Update: βœ…

Image