site-kit-wp: Adjust verbiage from 'Cancel' to 'Back' where user is presented an expected error message
Bug Description
While testing https://github.com/google/site-kit-wp/issues/433 I noticed during the error flow the verbiage on the lower call to action is labeled ‘Cancel’ although the user’s action was already canceled due to an error.
Steps to reproduce
- Adjust tag manager permissions after placing snipped on FE
- Run through Tag Manager service setup
Notice the expected error occurs and the user is presented a cancel button
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
- The “Cancel” link in the footer of the module set up wrapper (
ModuleSetup
) should use alternate text content in the case where the user is blocked from proceeding (such as when there is an existing tag that the user does not have access to) - In this case, the link should have the text content “Back”
- This should be implemented via a new piece of centralized infrastructure that could be used for every module. However, for now it should only be implemented specifically for the AdSense module, when an ad blocker is active and therefore prevents the setup.
Implementation Brief
- Using
assets/js/googlesitekit/data/utils.js
,- Update
createValidationSelector
, add a second parameter which is an object withnegate : false
as default value, which indicates whether to negate the boolean result or not.- Instead of returning
true
orfalse
, return the appopriate boolean value based onnegate
.
- Instead of returning
- Update
- Using
assets/js/googlesitekit/modules/create-module-store.js
,- Update
createModuleStore
:- to add another key,
validateIsSetupBlocked
for any options which is a function to validate whether module setup is terminally blocked from continuing. - If
requiresSetup
istrue
andvalidateIsSetupBlocked
is defined, callcreateValidationSelector
, withnegate: true
and add the results to the list of selectors. They should be:isSetupBlocked
for the safe selector__dangerousIsSetupBlocked
for the unsafe one.
- to add another key,
- Update
- All those changes above can be viewed in the following branch:
poc/1045-is-setup-blocked
- Using
assets/js/modules/adsense/datastore/base.js
,- When creating the module store, i.e when calling
Modules.createModuleStore
, definevalidateIsSetupBlocked
in the options object and using theadsense
data store.- check if there is an ad blocker active via the
isAdBlockerActive
selector. If this is the case, throw an error.
- check if there is an ad blocker active via the
- When creating the module store, i.e when calling
- Create
assets/js/components/setup/ModuleSetupFooter.js
which renders the same markup withingooglesitekit-setup__footer
inassets/js/components/setup/ModuleSetup.js
with the following changes:- It accepts the
module
prop. - Use
Grid
,Row
andCell
for the grid layout. - Query whether the setup is blocked via the
isSetupBlocked
selector. The module store name can be obtained viamodule.storeName
and since not all modules have this selector defined, it can be queried as such:
const isSetupBlocked = useSelect( ( select ) => select( module.storeName )?.isSetupBlocked?.() )
- If the selector returns
true
, displayBack
, else displayCancel
as it is currently.
- It accepts the
- Update
assets/js/components/setup/ModuleSetup.js
:- to use
ModuleSetupFooter
. - to use
Grid
,Row
andCell
for the grid layout.
- to use
Test Coverage
- No new tests to be added.
Visual Regression Changes
QA Brief
- Disconnect AdSense module.
- Enable your AdBlocker and proceed to setup AdSense module.
- Instead of displaying “Cancel”, the verbiage should be “Back” when the AdSense AdBlocker error is displayed.
- Disable your AdBlocker, the link should be: “Cancel”
Changelog entry
- Update the cancel button on the module setup form to have a
Back
label when the setup process can’t proceed.
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 20 (7 by maintainers)
@aaemnnosttv Good point that we can probably still move parts of this forward already. However, I think the only module-specific integration that we should cover in here for now is the AdSense setup part of having an ad blocker, as that will remain relevant in the future. The Tag Manager and Analytics cases where the setup is currently blocked will be drastically simplified, and therefore it’s not worth covering that here just to remove it again later. The AdSense setup flow itself will change quite a bit too.
I’ve therefore added a third point to the ACs that clarify that the actual implementation of that new API infrastructure should only be done for the ad blocker part of AdSense as part of this issue. @asvinb If that sounds good, can you please update the IB accordingly and remove all the parts regarding Tag Manager and Analytics? Other than that, this should be good to go.
@eclarke1 – the IB needs to be rewritten as we got off track after the initial review here but it can be done by anyone, using https://github.com/google/site-kit-wp/compare/poc/1045-is-setup-blocked as a starting point.
@aaemnnosttv Hmm, is it really overkill? I see how it may feel so since the goal of this issue is to only change the button label, but whether a module setup is blocked is something that in principle can be relevant for any module, so I think integrating the implementation into “core” makes sense. Right now we already have a need for it in GA and GTM, and you could argue a similar change would make sense in AdSense for the cases when the user is in a scenario where they just have to wait for AdSense review etc.
Hey @danielgent, can you look at this IB again outline the approach taken in the PR and the one that you’d recommend here? The existing IB text isn’t really a brief and seems more like discovery text used to outline the current/proposed flow, but I’m not quite sure which.
It’s worth noting that the linked branch doesn’t have the “Back” string translated, but passed directly into the rendered output as
'Back'
, not__( 'Back' )
.But it also looks like the child component is… causing a side effect that changes what the parent renders? This is a bit confusing—even if it’s a logic flow that doesn’t have bugs it’s a bit hard to understand. Maybe we want to store a piece of data in the
CORE_UI
data store (something we use as a sort of “catch-all” data store to store transitory values) that marks the text as needing to change? The current flow just strikes me as a bit roundabout. 🤷🏻♂️Let me know if you want to chat about it more or correct a mistake I made, maybe I’m missing something. 🙂