Create `SetupComponent` in `Ads` Module
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).
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
SetupComponentobject during module registration- A suitable
SetupMaincomponent should be created to reference above, see the existing GA4 component atassets/js/modules/analytics-4/components/setup/SetupMain.jsfor an idea of how this is configured. Note that the Ads Module component would be far simpler in nature, however. - Create a suitable
SetupFormcomponent to render within theSetupMaincomponent above (see Figma design)- Create the necessary
formcomponent with CTA to render and save a singleAds Conversion IDfield. - This field should confirm to the existing Ads options storage patterns, see #8251 for an example
- The field should contain the same validation and appearance logic as carried out in #8251, the input component therein should be reused
- Create the necessary
- 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
- A suitable
Implementation Brief
-
[x] Set
requiresSetup: trueinassets/js/modules/ads/datastore/base.js. -
[x] Create a new component
assets/js/modules/ads/components/setup/SetupMain.js:- [x] This component should simply load the
SetupFormcomponent with icon and header as in the Figma designs.
- [x] This component should simply load the
-
[x] As in #8344, migrate the
assets/js/modules/ads/components/common/AdsConversionIDTextField.jscomponent from analytics-4 and move theisValidAdsConversionIDvalidation into a new file:assets/js/modules/ads/utils/validation.js -
[x] Create a new component
assets/js/modules/ads/components/setup/SetupForm.js:- [x] This component should simply load the
AdsConversionIDTextFieldcomponent wrapped in adiv.googlesitekit-setup-module__inputs. - [x] This component should use the form and submitChanges structure as in
assets/js/modules/analytics/components/setup/SetupForm.jsto save changes before calling thefinishSetupprop function. - [x] Update the form as required to match the Figma designs.
- [x] This component should simply load the
-
[x] Import and pass the new
SetupMaincomponent to theSetupComponentkey in the module registration inassets/js/modules/ads/index.js. -
[x] Show the new style connection banner when the connection is complete.
Test Coverage
- [x] Move the validation tests for the
isValidAdsConversionIDvalidation function into a new test file:assets/js/modules/ads/utils/validation.test.js. - [x] Add a story for the new
SetupFormcomponent. - [x] Fix any broken VSTs
QA Brief
- Enable the
adsModulefeature 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.
ACs here look good, I added a link to the relevant Figma design in the ACs and a screenshot for context, but otherwise good. 👍🏻
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.
IB ✅
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
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:
Implementation:
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:
Implementation: 'Save' button
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
https://github.com/google/site-kit-wp/assets/125576926/7cf4d69e-477d-4626-8834-00dc80884501
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
https://github.com/google/site-kit-wp/assets/125576926/bcf99cb9-7f9b-42ac-8186-8d6f532eae41
@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.
I think we also want to revise the helper text but I'll ask @sigal-teller about this.
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.
(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)
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
Screenshots/videos for other items tested good
Issue 4 - Pass
________________________________________________________________________________
Error message no longer shifts down - Pass
https://github.com/google/site-kit-wp/assets/125576926/c3b682f3-8957-4fa3-8c42-07ad33c80532
Thanks @kelvinballoo, I've addressed this issue in a new PR and will put this to code review.
@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.
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.
-
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.
Moving this ticket over to 'Approval'.