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 SetupComponent in Ads Module #8346

Closed
13 tasks done
10upsimon opened this issue Mar 5, 2024 · 11 comments
Closed
13 tasks done

Create SetupComponent in Ads Module #8346

10upsimon opened this issue Mar 5, 2024 · 11 comments
Labels
Good First Issue Good first issue for new engineers javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P0 High priority Type: Feature New feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented Mar 5, 2024

Feature Description

As part of the work carried out in the new Ads Module is the need to create a SetupComponent that will facilitate initial setup of the Ads module for both new and existing users of Site Kit. This setup module is required in order to connect the module and make available the SettingsView and SettingsEdit component(s).

Screenshot 2024-03-05 at 10 56 54

This SetupComponent should contain the necessary main SetupMaincontainer component as well as the SetupForm component and applicable form with fields. At present, the only field required is the Ads Conversion ID field, as it appears in the SettingsView and Settings Edit components, see #8251

See the applicable design doc here:
https://docs.google.com/document/d/1APuSv95bf62uhzlaFlW6jPrzPKy1avpRYd9W1MSAAJo/edit?resourcekey=0-UuynlcUz9CoubgldR6Z5sg#heading=h.lzbos67ly9f6

See the applicable setup view Figma design here:
https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=121-1080&mode=design&t=sbmhawdoXe4KNC0S-0

See the applicable post-setup Figma design here:
https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=60-788&mode=design&t=sbmhawdoXe4KNC0S-0


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

Acceptance criteria

  • A new component should be created that maps to the SetupComponent object during module registration
    • A suitable SetupMain component should be created to reference above, see the existing GA4 component at assets/js/modules/analytics-4/components/setup/SetupMain.js for an idea of how this is configured. Note that the Ads Module component would be far simpler in nature, however.
    • Create a suitable SetupForm component to render within the SetupMain component above (see Figma design)
    • The Ads module should be able to be successful set up and be connected following completion of this form
      • Upon successful setup, the user should be redirected to the dashboard with the applicable notification banner as shown in the Figma design here
    • Saving states and indicators should be used accordingly, as per the existing Site Kit design patterns

Implementation Brief

  • Set requiresSetup: true in assets/js/modules/ads/datastore/base.js.

  • Create a new component assets/js/modules/ads/components/setup/SetupMain.js:

    • This component should simply load the SetupForm component with icon and header as in the Figma designs.
  • As in Add the Ads module settings screens #8344, migrate the assets/js/modules/ads/components/common/AdsConversionIDTextField.js component from analytics-4 and move the isValidAdsConversionID validation into a new file: assets/js/modules/ads/utils/validation.js

  • Create a new component assets/js/modules/ads/components/setup/SetupForm.js:

    • This component should simply load the AdsConversionIDTextField component wrapped in a div.googlesitekit-setup-module__inputs.
    • This component should use the form and submitChanges structure as in assets/js/modules/analytics/components/setup/SetupForm.js to save changes before calling the finishSetup prop function.
    • Update the form as required to match the Figma designs.
  • Import and pass the new SetupMain component to the SetupComponent key in the module registration in assets/js/modules/ads/index.js.

  • Show the new style connection banner when the connection is complete.

Test Coverage

  • Move the validation tests for the isValidAdsConversionID validation function into a new test file: assets/js/modules/ads/utils/validation.test.js.
  • Add a story for the new SetupForm component.
  • Fix any broken VSTs

QA Brief

  • Enable the adsModule feature flag.
  • Go to the Site Kit settings and connect the new Ads module using the new module setup flow.
  • Once connected the user should be redirected to the plugin dashboard with the banner confirming the connection:
    • This banner should be dismissed by clicking "Got It".
  • Test disconnecting the module by going to the plugin settings, opening the Ads module accordion, clicking "Edit", then "Disconnect":
    • The disconnect modal warning should show with the features list.
    • The module should be able to be disconnected.

Changelog entry

  • Add setup flow to Ads Module.
@10upsimon 10upsimon added Type: Feature New feature P0 High priority javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues labels Mar 5, 2024
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Mar 5, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented Mar 5, 2024

ACs here look good, I added a link to the relevant Figma design in the ACs and a screenshot for context, but otherwise good. 👍🏻

@benbowler benbowler self-assigned this Mar 5, 2024
@benbowler
Copy link
Collaborator

Added IB and set as blocking #8251.

In the interest of speed, I created a draft PR here, that can be used as the basis of this work once the IB is approved.

@benbowler benbowler removed their assignment Mar 5, 2024
@tofumatt tofumatt self-assigned this Mar 5, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented Mar 5, 2024

IB ✅

@tofumatt tofumatt assigned benbowler and unassigned tofumatt Mar 5, 2024
@benbowler benbowler removed their assignment Mar 7, 2024
@10upsimon 10upsimon assigned 10upsimon and benbowler and unassigned 10upsimon Mar 7, 2024
@benbowler benbowler assigned 10upsimon and unassigned benbowler Mar 11, 2024
@10upsimon 10upsimon removed their assignment Mar 19, 2024
@tofumatt tofumatt assigned tofumatt and benbowler and unassigned tofumatt Mar 19, 2024
@benbowler benbowler assigned tofumatt and unassigned benbowler Mar 20, 2024
@tofumatt tofumatt removed their assignment Mar 20, 2024
@wpdarren wpdarren added the Good First Issue Good first issue for new engineers label Mar 25, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Apr 1, 2024

QA Update ❌

  • Tested on WP 6.4.3 and PHP 8.0 on dev branch
  • Tested on MacOS Ventura Chrome, Firefox and safari.
  • Issues identified are referenced below:

ISSUE 1:
On the disconnect notice, the 2 bullets should start with a small letter case, based on figma.

Screenshots Screenshot 2024-04-01 at 19 04 48
____________________________________________________________________________

ISSUE 2:
The warning icon is different between what we implemented and figma.
Also, the implemented 'Conversion Tracking ID' title on top of the input box doesn't exist in figma

Screenshots

Figma:

Screenshot 2024-04-01 at 19 31 01

Implementation:

Screenshot 2024-04-01 at 19 31 23
____________________________________________________________________________

ISSUE 3:
When we just click on 'Edit', the CTA we firstly see is 'Save'.
There is no reference of 'Save' button on Figma. It's just 'Confirm changes'.
I assume this is fine because other sections are constructed in that way.

Screenshots

Figma:

Screenshot 2024-04-01 at 19 35 55

Implementation: 'Save' button

Screenshot 2024-04-01 at 19 34 27
____________________________________________________________________________

ISSUE 4:
After we disconnect the Ads module and setup again, the 'Complete setup' button is disabled (even if it's a valid value) until we update the conversion value.
Video is attached for reference

Video
Enable.confirm.setup.mov
____________________________________________________________________________

ISSUE 5:
The error msg shifts down/up based on whether it's the first character of the Conversion ID or more.
Video is attached for easy reference. It should be a consistent 8px below the input field.

Video & Photos
Error.message.shifts.mov
Screenshot 2024-04-01 at 22 54 22

@aaemnnosttv
Copy link
Collaborator

@kelvinballoo

ISSUE 1:
On the disconnect notice, the 2 bullets should start with a small letter case, based on figma.

These should use sentence case as they do now for consistency with other modules. (Figma should be updated here)

ISSUE 2:
The warning icon is different between what we implemented and figma.

The warning icon used is the same we use in a few other places so it is correct for now. We should update Figma and/or open a separate issue to update this in all places for consistency.

Also, the implemented 'Conversion Tracking ID' title on top of the input box doesn't exist in figma

This should be fixed 👍 @benbowler I found there is a noLabel prop on the TextField but it doesn't seem to do what I'd expect. We probably still want the label to be there for a11y but not visible which is what I think that prop is for.

ISSUE 3:
When we just click on 'Edit', the CTA we firstly see is 'Save'.
There is no reference of 'Save' button on Figma. It's just 'Confirm changes'.
I assume this is fine because other sections are constructed in that way.

This is not specific to the Ads module and is handled in a centralized way across all modules. Okay to be leave as-is.

ISSUE 4:
After we disconnect the Ads module and setup again, the 'Complete setup' button is disabled (even if it's a valid value) until we update the conversion value.

I believe this is related to the settings not being cleared on deactivation. It should be addressed now via #8449.

The error msg shifts down/up based on whether it's the first character of the Conversion ID or more.

I didn't see this in my testing and your videos aren't working for me unfortunately.


One other thing I noticed is that the setup loads in an invalid input/warning state which shouldn't happen until someone enters something invalid.
image

I think we also want to revise the helper text but I'll ask @sigal-teller about this.

@benbowler
Copy link
Collaborator

Thanks for your responses @aaemnnosttv, the setup flow has been created in collaboration with @sigal-teller and Maria on Slack. Based on your comments I believe there are no additional changes to make.

RE this comment:

Also, the implemented 'Conversion Tracking ID' title on top of the input box doesn't exist in figma

This label does exist in Figma here for eg., we discussed that it should exist to be consistent with other MUI inputs across the app and for accessibility.

@benbowler benbowler assigned kelvinballoo and unassigned benbowler Apr 2, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented Apr 2, 2024

(Moving to QA as this was meant to be moved there but placed in Code Review by mistake, see Slack thread here: https://10up.slack.com/archives/CBKKQEBR9/p1712068179227499)

@kelvinballoo
Copy link
Collaborator

QA Update ❌

Thanks all.
Noted on issues 1-3 that figma needs updating and we are mirroring based on the other modules we already have.

Did a deeper check on the following:

  • Issue 2 - I don't know where I saw the figma not showing the 'Conversion Tracking ID' on the input box but I can now see it. It could be that figma as been updated. Since it's consistent with the design, marking this as pass.
  • Issue 4 - This is fixed under 8449. The value in the field is blanked and it makes sense to have the disabled icon then.
  • Issue 5 - This appears to have been fixed. It could have been fixed based on another ticket.

New Issue
I am adding one more item to the list as ISSUE 6. I don't know if it's documented or being fixed elsewhere, but here's the issue:

  • After having saved a valid Conversion Tracking ID, I go in to edit it again at the Ads module and it will prompt the error msg even if the ID is valid.

(Note: If it's documented elsewhere, we can move this to approved)

Screenshot for Issue 6

Upon first loading the edit mode, the message prompt will appear even if it's a valid ID

Screenshot 2024-04-03 at 12 50 29
________________________________________________________________________________
Screenshots/videos for other items tested good

Issue 4 - Pass

Screenshot 2024-04-03 at 12 46 19

________________________________________________________________________________

Error message no longer shifts down - Pass

https://github.com/google/site-kit-wp/assets/125576926/c3b682f3-8957-4fa3-8c42-07ad33c80532

@benbowler
Copy link
Collaborator

Thanks @kelvinballoo, I've addressed this issue in a new PR and will put this to code review.

@benbowler benbowler removed their assignment Apr 3, 2024
@kuasha420 kuasha420 assigned kuasha420 and benbowler and unassigned kuasha420 Apr 3, 2024
@benbowler benbowler assigned kuasha420 and unassigned benbowler Apr 3, 2024
@kuasha420
Copy link
Collaborator

@kelvinballoo Issue 6 should now be fixed. Also, in Ads setup, the "Complete Setup" button will be disabled until a valid id has been put in. Initially, when the Setup screen is opened, the field will be empty but in normal non error state. But once user interact with it, the validation will kick in. You can test this by putting something in the field and then clearing it up, the field should go into error state and the "Complete setup" button will be disabled.

Back to you for another pass.

@kuasha420 kuasha420 assigned kelvinballoo and unassigned kuasha420 Apr 3, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks @kuasha420 @benbowler

Retested issue 6 as follows and all are as expected:

  • After saving a valid conversion ID, and going back to edit it, there will be no error prompt on the first load of the edit mode page.

    Screenshot 2024-04-04 at 13 56 00
  • During setup as well, the 'Complete setup' button remains disabled on first load. Only when the user keys in a value that it will be enabled. Removing the value will make the button disabled again. This is as expected.

    Screenshot 2024-04-04 at 13 57 51

Moving this ticket over to 'Approval'.

@kelvinballoo kelvinballoo removed their assignment Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P0 High priority Type: Feature New feature
Projects
None yet
Development

No branches or pull requests

7 participants