Implement Ads Conversion ID Field in `Ads` Module `SettingsView` and `SettingsEdit` Components

[10upsimon]

Feature Description

As part of the newly created Ads module infrastructure (see issue #8225), comes the need to add the Ads Conversion ID field to the Ads module settings screen. This will need to be visible in the SettingsView and SettingsEdit, along with the required SettingsForm component. This is, however, reliant on the completion of the Ads module datastore, see issue #8226.

See the relevant Figma designs here (series of designs to the right of the linked view): https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=62-2205&mode=design&t=pqxmsz2Yu9IxNuMt-0

See the relevant area of the Ads Module Design Doc here: https://docs.google.com/document/d/1APuSv95bf62uhzlaFlW6jPrzPKy1avpRYd9W1MSAAJo/edit?resourcekey=0-UuynlcUz9CoubgldR6Z5sg#heading=h.lzbos67ly9f6

---

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

Acceptance criteria

• The Ads Conversion ID field should be implemented for View & Edit views within the Ads Module Settings section. These consist of the following component (and files):
  • assets/js/modules/ads/components/settings/SettingsEdit.js (for edit view)
  • assets/js/modules/ads/components/settings/SettingsForm.js (for edit view form field controls and saving logic etc)
  • assets/js/modules/ads/components/settings/SettingsView.js (for generic settings view)
• Within the SettingsView component of the Ads module settings, the applicable label and textual value should be implemented. The value of the saved Ads Conversion ID field should show, if saved, or be blank.
• Within the SettingsEdit, the necessary SettingsForm component should be created and rendered during edit. This should render the applicable input field that accepts a value for the Ads Conversion ID field value. Upon saving, this value should be correctly updated in the Ads datastore, and be retrievable as needed by associated logic.
• The necessary helper text/description should be rendered below the text input field, as per the Figma design, see https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=13-5713&mode=design&t=eV3r3gmTh4hXjm7K-0
  • The applicable inline error/warning message should be displayed when the user clears the field of a value entirely, as per the Figma design at https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=209-1765&mode=design&t=SG71Q7xEJiuX3gFP-0
  • This error/warning should be delayed by a suitable amount of time (~500ms) in case the user is clearing the value in a bid to add a new value
  • The error/notice does not need to prevent the user from saving the empty value, and is informatory only

Implementation Brief

• [ ] Settings components listed in AC, have been added in #8225
• [x] Update assets/js/modules/ads/components/settings/SettingsForm.js
  • Add Ads conversion id field bellow the StoreErrorNotices component, you can migrate the existing field https://github.com/google/site-kit-wp/blob/fd50e25181653aa4ed27db16f4742c2c9667d9e8/assets/js/modules/analytics-4/components/settings/SettingsForm.js#L52 to the assets/js/modules/ads/components/common/AdsConversionIDTextField.js
  • You can wrap it with div.googlesitekit-setup-module__inputs
• [x] In assets/js/modules/ads/components/common/AdsConversionIDTextField.js
  • Adapt the field to pull/save data into the MODULE_ADS datastore
  • Adapt the content to match the one in figma design linked in AC, and some styling if any is needed
  • Add local state, say [isEmptyID, setIsEmptyID], to control displaying the empty value error
  • In onChange callback :
    • Check if value is empty
    • If it is empty, use useDebounce hook to update setEmptyValue after 500ms
    • Either way update the the value of the adsConversionID setting to the store
  • In helperText prop of TextField component, use isEmptyID to determine if error should be displayed, using the content from figma

• [x] SettingsView component already implements the logic to render ads conversion ID field, update label if needed
• [x] Note, for save/update button we are currently using label Apply changes https://github.com/google/site-kit-wp/blob/fd50e25181653aa4ed27db16f4742c2c9667d9e8/assets/js/components/settings/SettingsActiveModule/Footer.js#L203 and in the figma design, it is now Confirm changes, so update the referenced line with new label

Test Coverage

• Add stories for the SettingsForm component

QA Brief

• [ ] Enable the adsModule feature flag.
• [ ] Connect the Ads module
• [ ] Visit the settings and save and update the Ad Conversion ID. Note from the AC: The error/notice does not need to prevent the user from saving the empty value, and is informatory only

Changelog entry

• Add the Conversion Tracking ID field to the Ads module's Setup and Settings screens.

[eugene-manuilov]

AC 🌶️

[10upsimon]

@zutigrm I have updated the AC as per internal comms, the last point and it's sub points have been added to reflect the requirement for an inline error/warning notice should the user clear the field of the ads conversion ID entirely.

[tofumatt]

useInterval will run a function every 500ms, but not actually 500ms after the user has updated the text. I think you probably want some kind of useDebounce or similar, to update only 500ms after the final change event.

Otherwise you could change the content and if it's running every 500ms you might actually trigger it immediately, which would be jarring.

Add js tests and stories for the Settings components

Can you outline the JS tests you would add here? What should we be testing for?

[zutigrm]

Thanks @tofumatt for the feedback

useInterval will run a function every 500ms, but not actually 500ms after the user has updated the text. I think you probably want some kind of useDebounce or similar, to update only 500ms after the final change event.

Ah yes, good point! Thanks, switched to the useDebounce

Can you outline the JS tests you would add here? What should we be testing for?

Originally I though including js test for the error state, but as I can see we don't do tests for SettingsForm component, only stories, so I removed tests suggestion

[tofumatt]

IB ✅

[samr874]

@tofumatt what is the status of this issue tell me does it need to get worked upon?

[mxbclang]

Hi @samr874 ! This is currently in progress with our internal team, so it does not need any work right now. If you're interested in working on Site Kit issues, please check out our wiki to get started. Thanks!

[samr874]

@bethanylang will start working on the issues are those issues currently worked upon by internal team?

[mxbclang]

@samr874 As a general rule, any issue that is assigned to someone is already being worked on.

[kelvinballoo]

QA Update ❌

• Tested on WP 6.4.3 and PHP 8.0 on dev branch
• Tested on MacOS Ventura Chrome, Firefox and safari, Edge (Windows 11).
• Also tested Mobile and Tablet breakpoint on Chrome Emulator.
• Issues identified are referenced below:

ISSUE 1: AC states that: The applicable inline error/warning message should be displayed when the user clears the field of a value entirely, as per the Figma design...

However, the message also appears when the field is not entirely blank. e.g. 3 characters as per the screenshot

Screenshot

____________________________________________________________________________________

ISSUE 2: For the following AC: This error/warning should be delayed by a suitable amount of time (~500ms) in case the user is clearing the value in a bid to add a new value

Is there a way to test this? I tried to put the network on 3G but even then, that is hard to validate.

---

ISSUE 3: When I save 'AW-123', it will show as '123' only in View mode.

Screenshot

____________________________________________________________________________________

ISSUE 4: There are some designs adjustments based on figma for the edit mode. Details are in the attached screenshot.

Screenshot

____________________________________________________________________________________

ISSUE 5: In edit mode, there should be a 6px gap (not 8px) between the input field and the warning msg. Refer to the attached figma image.

Screenshot

[benbowler]

Thanks @kelvinballoo, I've addressed all comments except point 4:

There are some designs adjustments based on figma for the edit mode.

My thoughts is that pixel perfect changes to the standard MUI text area would make the input here inconsistent with all other MUI text areas in the app, @aaemnnosttv can you confirm if you would like the pixel perfect changes to the MUI text area here or that we are good to go as it.

[tofumatt]

The design here is good-to-go as-is, just for the record 🙂

[kelvinballoo]

QA Update ✅

Thanks @benbowler .

Retested as follows:

ISSUE 1: ✅ Now, the error message doesn't appear on the 2nd or 3rd character. This is as expected.

Screenshots

__________________________________________________________________________________________________

ISSUE 2: ✅ Although I can't measure this accurately to 500ms, I can now feel there is a delay before the error message appears. This is as expected

Screencast

https://github.com/google/site-kit-wp/assets/125576926/971affb7-8cdc-4702-a389-d3d0404d530f

__________________________________________________________________________________________________

ISSUE 3: ✅ When I save 'AW-123', it now shows as 'AW-123' only in View mode and not '123' only.

Screenshot

__________________________________________________________________________________________________

ISSUE 4: ✅ Noted that we won't be applying changes for this as it's being consistent with other areas of the app, even though figma says otherwise.

---

ISSUE 5: ✅ The padding is now 6px

Screenshots

Implementation

Marking this ticket as Pass and moving to 'Approval'