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
Implement Ads Conversion ID Field in Ads
Module SettingsView
and SettingsEdit
Components
#8251
Comments
AC 🌶️ |
@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. |
Otherwise you could change the content and if it's running every 500ms you might actually trigger it immediately, which would be jarring.
Can you outline the JS tests you would add here? What should we be testing for? |
Thanks @tofumatt for the feedback
Ah yes, good point! Thanks, switched to the
Originally I though including js test for the error state, but as I can see we don't do tests for |
IB ✅ |
@tofumatt what is the status of this issue tell me does it need to get worked upon? |
@bethanylang will start working on the issues are those issues currently worked upon by internal team? |
@samr874 As a general rule, any issue that is assigned to someone is already being worked on. |
QA Update ❌
ISSUE 1: However, the message also appears when the field is not entirely blank. e.g. 3 characters as per the screenshot ____________________________________________________________________________________ISSUE 2: 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: ISSUE 4: ISSUE 5: |
Thanks @kelvinballoo, I've addressed all comments except point 4:
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. |
The design here is good-to-go as-is, just for the record 🙂 |
QA Update ✅Thanks @benbowler . Retested as follows: ISSUE 1: ✅ ISSUE 2: ✅ Screencast500ms.movISSUE 3: ✅ ISSUE 4: ✅ ISSUE 5: ✅ Marking this ticket as Pass and moving to 'Approval' |
Feature Description
As part of the newly created
Ads
module infrastructure (see issue #8225), comes the need to add theAds Conversion ID
field to the Ads module settings screen. This will need to be visible in theSettingsView
andSettingsEdit
, along with the requiredSettingsForm
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
Ads Conversion ID
field should be implemented for View & Edit views within theAds
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)SettingsView
component of theAds
module settings, the applicable label and textual value should be implemented. The value of the savedAds Conversion ID
field should show, if saved, or be blank.SettingsEdit
, the necessarySettingsForm
component should be created and rendered during edit. This should render the applicable input field that accepts a value for theAds Conversion ID
field value. Upon saving, this value should be correctly updated in theAds
datastore, and be retrievable as needed by associated logic.Implementation Brief
Ads
module codebase structure inassets/js/modules
#8225assets/js/modules/ads/components/settings/SettingsForm.js
StoreErrorNotices
component, you can migrate the existing fieldsite-kit-wp/assets/js/modules/analytics-4/components/settings/SettingsForm.js
Line 52 in fd50e25
assets/js/modules/ads/components/common/AdsConversionIDTextField.js
div.googlesitekit-setup-module__inputs
assets/js/modules/ads/components/common/AdsConversionIDTextField.js
MODULE_ADS
datastore[isEmptyID, setIsEmptyID]
, to control displaying the empty value erroronChange
callback :useDebounce
hook to updatesetEmptyValue
after 500msadsConversionID
setting to the storehelperText
prop ofTextField
component, useisEmptyID
to determine if error should be displayed, using the content from figmaSettingsView
component already implements the logic to render ads conversion ID field, update label if neededApply changes
site-kit-wp/assets/js/components/settings/SettingsActiveModule/Footer.js
Line 203 in fd50e25
Confirm changes
, so update the referenced line with new labelTest Coverage
SettingsForm
componentQA Brief
adsModule
feature flag.Changelog entry
The text was updated successfully, but these errors were encountered: