Create the Ads module setup flow. #8348

benbowler · 2024-03-05T12:22:15Z

Summary

Addresses issue:

Create SetupComponent in Ads Module #8346

Relevant technical choices

I build out the module setup flow as in the IB, as part of this work I had to update how the authentication notification shows the banner on the dashboard to use the new SubtleNotification instead of the BannerNotification. Finally while I was working on this ticket I added the features to the module registration to match the mockups when disconnecting.

I updated the settings field title to "Conversion ID" based on a comment from @aaemnnosttv in the sprint meeting 6 March 2024.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2024-03-06T16:03:02Z

Build files for 65f6c0d are ready:

Download development build
Download production build

…n on set up completion.

github-actions · 2024-03-07T10:55:40Z

Size Change: +3.73 kB (0%)

Total Size: 1.41 MB

Filename	Size	Change
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	52.4 kB	+68 B (0%)
`./dist/assets/js/32-********************.js`	930 B	+1 B (0%)
`./dist/assets/js/34-********************.js`	3.12 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-activation-********************.js`	23.8 kB	+70 B (0%)
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	57.3 kB	+410 B (+1%)
`./dist/assets/js/googlesitekit-adminbar-********************.js`	34.1 kB	+53 B (0%)
`./dist/assets/js/googlesitekit-api-********************.js`	10 kB	-3 B (0%)
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.86 kB	+3 B (0%)
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	10 kB	-68 B (-1%)
`./dist/assets/js/googlesitekit-data-********************.js`	2.18 kB	-4 B (0%)
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.11 kB	+6 B (0%)
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	18.3 kB	-3 B (0%)
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	10 kB	+10 B (0%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	24.5 kB	+126 B (+1%)
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	65.3 kB	+276 B (0%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	90.1 kB	+129 B (0%)
`./dist/assets/js/googlesitekit-modules-ads-********************.js`	16.6 kB	+1.16 kB (+8%)	🔍
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	111 kB	+345 B (0%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	109 kB	+42 B (0%)
`./dist/assets/js/googlesitekit-modules-********************.js`	21.7 kB	-18 B (0%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	22.5 kB	+17 B (0%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	57.7 kB	+104 B (0%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	29.8 kB	+44 B (0%)
`./dist/assets/js/googlesitekit-settings-********************.js`	59 kB	+284 B (0%)
`./dist/assets/js/googlesitekit-splash-********************.js`	72 kB	+298 B (0%)
`./dist/assets/js/googlesitekit-user-input-********************.js`	46.8 kB	+281 B (+1%)
`./dist/assets/js/googlesitekit-vendor-********************.js`	316 kB	-1 B (0%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	33.2 kB	-39 B (0%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	59.5 kB	+132 B (0%)
`./dist/assets/js/runtime-********************.js`	1.3 kB	+3 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11.2 kB
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	7.47 kB
`./dist/assets/js/29-********************.js`	2.8 kB
`./dist/assets/js/30-********************.js`	2.28 kB
`./dist/assets/js/31-********************.js`	3.72 kB
`./dist/assets/js/33-********************.js`	888 B
`./dist/assets/js/analytics-advanced-tracking-********************.js`	778 B
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.09 kB
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.92 kB
`./dist/assets/js/googlesitekit-polyfills-********************.js`	378 B

_{compressed-size-action}

10upsimon

@benbowler thank you for this, code mostly LGTM aside from an alignment issue with the presence of Ads Conversion ID input notice for when there is no value entered as yet, I do not believe that is supposed to be in the setup screen as per the Figma mock: https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=121-1080&mode=design&t=s7FMIhQebaLXcUvT-0

It only appears to exist in the component view once the module is set up, see the Figma mock at https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=209-1765&mode=design&t=s7FMIhQebaLXcUvT-0

benbowler · 2024-03-11T18:08:17Z

@10upsimon moving this back to CR with you after reading Sigals Slack message about the validation message and including validation here in the setup flow as in the settings pages.

…odule-setup.

benbowler · 2024-03-18T15:32:02Z

FYI I updated some of the copy in the Ads Settings branch in this commit.

10upsimon

@benbowler I noticed one small nitpick WRT banner naming convention but keen to hear your thoughts.

10upsimon · 2024-03-19T08:56:04Z

assets/js/components/notifications/SetupSuccessSubtleNotification.js

@benbowler I was thinking about this for a while, and it may seem like a nit pick comment, but if this banner is only related to ads (and it is, given the textual nature of it) I feel we may want to be a bit more explicit in terms of naming convention here. The file name and its location do not really elude to the fact that it is an ads only banner.

I think it may be worth renaming this to AdsSetupSuccessSubtleNotification or similar, what do you think?

As per Slack comms, the thought from @benbowler here was that in future other modules may use this style, and conditional output will simply live within this same component.

10upsimon · 2024-03-19T08:56:38Z

assets/js/components/notifications/SetupSuccessSubtleNotification.stories.js

The same applies to the stories file as I outlined above IRO a more ads explicit naming convention here.

10upsimon

Thanks @benbowler this seems good to go IMO.

tofumatt

Looks good, just a few minor copy issues; I'll make the changes and merge 👍🏻

assets/js/modules/ads/components/setup/SetupForm.js

assets/js/modules/ads/index.js

tofumatt

Just needs the merge conflicts resolved and then we're good-to-go 😄

…odule-setup.

benbowler mentioned this pull request Mar 5, 2024

Create SetupComponent in Ads Module #8346

Closed

13 tasks

benbowler marked this pull request as ready for review March 6, 2024 15:56

benbowler added 2 commits March 7, 2024 10:43

Create the foundation of the Ads module setup flow.

abd4960

Update the SetupForm to match the mockups and add a SubtleNotificatio…

4b8c3a8

…n on set up completion.

benbowler force-pushed the feature/8346-ads-module-setup branch from be4fe9d to 4b8c3a8 Compare March 7, 2024 10:48

Add index exports for common and setup components.

c3ac24d

benbowler changed the title ~~DRAFT: Create the foundation of the Ads module setup flow.~~ Create the foundation of the Ads module setup flow. Mar 7, 2024

10upsimon requested changes Mar 7, 2024

View reviewed changes

Merge remote-tracking branch 'origin/develop' into feature/8346-ads-m…

1cfe811

…odule-setup.

benbowler changed the title ~~Create the foundation of the Ads module setup flow.~~ Create the Ads module setup flow. Mar 12, 2024

hussain-t mentioned this pull request Mar 15, 2024

Create Ads Module Site Health Checks #8245

Closed

2 tasks

10upsimon requested changes Mar 19, 2024

View reviewed changes

10upsimon self-requested a review March 19, 2024 09:04

10upsimon approved these changes Mar 19, 2024

View reviewed changes

tofumatt approved these changes Mar 20, 2024

View reviewed changes

assets/js/modules/ads/components/setup/SetupForm.js Outdated Show resolved Hide resolved

assets/js/modules/ads/index.js Outdated Show resolved Hide resolved

Update copy in features and form.

e248ee6

tofumatt approved these changes Mar 20, 2024

View reviewed changes

tofumatt reviewed Mar 20, 2024

View reviewed changes

benbowler and others added 2 commits March 20, 2024 14:34

Merge remote-tracking branch 'origin/develop' into feature/8346-ads-m…

ffca2ef

…odule-setup.

Update VRT images with copy change.

65f6c0d

tofumatt merged commit 12bb2a8 into develop Mar 20, 2024
14 checks passed

tofumatt deleted the feature/8346-ads-module-setup branch March 20, 2024 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create the Ads module setup flow. #8348

Create the Ads module setup flow. #8348

benbowler commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024 •

edited

Loading

10upsimon left a comment •

edited

Loading

benbowler commented Mar 11, 2024

benbowler commented Mar 18, 2024

10upsimon left a comment

10upsimon Mar 19, 2024 •

edited

Loading

10upsimon Mar 19, 2024

10upsimon Mar 19, 2024

10upsimon left a comment

tofumatt left a comment

tofumatt left a comment

Create the Ads module setup flow. #8348

Create the Ads module setup flow. #8348

Conversation

benbowler commented Mar 5, 2024 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Mar 6, 2024 • edited Loading

github-actions bot commented Mar 7, 2024 • edited Loading

10upsimon left a comment • edited Loading

Choose a reason for hiding this comment

benbowler commented Mar 11, 2024

benbowler commented Mar 18, 2024

10upsimon left a comment

Choose a reason for hiding this comment

10upsimon Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

10upsimon Mar 19, 2024

Choose a reason for hiding this comment

10upsimon Mar 19, 2024

Choose a reason for hiding this comment

10upsimon left a comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

benbowler commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024 •

edited

Loading

10upsimon left a comment •

edited

Loading

10upsimon Mar 19, 2024 •

edited

Loading