google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 284 forks source link

No setup option for users who have already configured a message #7289

Closed mxbclang closed 3 months ago

mxbclang commented 1 year ago

Feature Description

As reported by @wpdarren in Bug Bash:

We could simplify the set up for the case where a user already has set up an ABR message in AdSense by providing a secondary option to skip this part, similar to the post-click version of this today (2b).

2a) Initial state

image

2b) Post-click

image

That is, we could change 2a) to be the reverse of 2b), like so

image


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

Acceptance criteria

Implementation Brief

In assets/js/modules/adsense/components/setup/AdBlockingRecoveryApp/steps/CreateMessageStep.js:

Test Coverage

QA Brief

Changelog entry

techanvil commented 1 year ago

Hi @kuasha420, thanks for drafting the AC here.

It's a small point, but I would suggest that create_message and confirm_message are quite implementation-specific terms and are a little out of place in the AC. It is pretty clear what they refer to, but it's preferable to use language that's more descriptive in terms of the flow rather than referring to these underlying implementation details.

Secondly, the final point is a little unclear in its wording and could with some clarification (I think it's just a case of updating it to read "track the ... event" instead of "share"?). Also, as the intent here does appear to be to track this event with a new label, we should keep @marrrmarrr in the loop here with a view to confirming the event variant and keeping track of it in the events sheet.

kuasha420 commented 1 year ago

@techanvil Thank you for your feedback.

  1. You're right about the create_message and confirm_message are slightly implementation specific - but as both of them are part of step 2, saying step 2 before clicking and after clicking the primary CTA sounds more vague than this to me, and I think this make sense here.

  2. I've updated the wording here, and will let Mariya know about this so she can update the Event tracking sheet once the AC is approved.

Cheers.

techanvil commented 1 year ago

Hey @kuasha420, thanks for following up here.

  1. You're right about the create_message and confirm_message are slightly implementation specific - but as both of them are part of step 2, saying step 2 before clicking and after clicking the primary CTA sounds more vague than this to me, and I think this make sense here.

I do take your point, but thinking about reading the AC from another perspective, say from a QA point of view, I'm still not sure it's all that clear. Maybe another way of describing it would be to talk about Step 2 before and after the CTA click, and also link in to Storybook to illustrate the point. What do you think?

  1. I've updated the wording here, and will let Mariya know about this so she can update the Event tracking sheet once the AC is approved.

Thanks for updating the wording and letting Mariya know about the event. Sorry if this sounds pedantic, though, but I would still suggest the wording could be a little clearer - just updating it to start with "The newly added CTA should track an event using the existing confirm_message_ready event name" would help to make it very clear its supposed to track a new (well, a variant of an existing) event.

kuasha420 commented 1 year ago

@techanvil Thank you for your feedback and the idea about linking storybook. I've updated the AC accordingly. Please have a look. Cheers.

techanvil commented 1 year ago

Thanks @kuasha420, this LGTM - I have made one small further adjustment to make it explicit that that new CTA should have the same text as the primary CTA, i.e. "My message is ready".

AC :white_check_mark:

kuasha420 commented 1 year ago

@techanvil Thank you, I'm a bit confused by the "that new CTA should have the same text as the primary CTA", as far as I remember, even though that's the case in the mock screenshot, we decided to use the more explicit wording as described in the AC, as discussed in one of our AC sync. Maybe @aaemnnosttv would be able to confirm.

Cheers.

techanvil commented 1 year ago

Ah, my apologies @kuasha420. I had intended to save another back and forth but should have sent it back to you rather than making this change myself.

I had thought based on a previous discussion about CTAs that we were aiming to avoid relatively long text content within them. But, if we are indeed using the text "I have already created and published a message in AdSense" as the CTA, I think the AC could be a touch more explicit to make this entirely clear, what with the mockup showing a different label for the button.

I've tweaked it myself, please see what you think. Maybe we should then ask Evan to do the final ACR based on your comment above.

kuasha420 commented 1 year ago

Thank you @techanvil. AC LGTM. Assigning this over to Evan for final pass. Cheers.

aaemnnosttv commented 9 months ago

IMO the more explicit wording in the CTA is redundant in the context since it already says that right above. Unless this has been called for by @marrrmarrr – I'd suggest we keep it as initially proposed.

techanvil commented 9 months ago

@kuasha420, does that answer your question or do you want to check with Mariya on this one?

kuasha420 commented 9 months ago

@aaemnnosttv @techanvil From our conversation in AC sync with @marrrmarrr, I remember her saying we should over communicate this to the user, specially until we have the API to actually know if user has actually created and published the message. I will send her ping in the open channel to make sure I'm remembering things correctly and get back on this, leave this with me for now. Cheers.

marrrmarrr commented 9 months ago

@kuasha420 @aaemnnosttv perhaps something shortened like "I published my message" to clarify that they completed the last required step would work?

tofumatt commented 9 months ago

ACs here sound good 👍🏻

derweili commented 9 months ago

We have another issues here https://github.com/google/site-kit-wp/issues/7338 that refactors the same component. Unsure how to to write the IB here knowing that the relevant function will be changed by this other issue.

@bethanylang does it make sense to either set a dependency here or combine the two tasks?

mxbclang commented 9 months ago

Thanks for flagging, @derweili! @kuasha420 Can you please take a look at this versus #7338 and determine next steps here? Assigning this over to you for now.

kuasha420 commented 9 months ago

@derweili Thanks for flagging, the flagged issues are fairly intertwined, and since the other one already seem to have a partial IB, I'm making this one depend on #7338 to make things easier here. Cheers.

techanvil commented 9 months ago

Thanks @hussain-t, this IB looks :white_check_mark:, but before moving to the EB I've asked @marrrmarrr on Slack to confirm the new GA event we've got specced here.

techanvil commented 9 months ago

The GA event is confirmed, I'm moving this to EB :+1:

mohitwp commented 3 months ago

QA Update ✅

![image](https://github.com/google/site-kit-wp/assets/94359491/e17b8632-2c2b-44b6-8012-9b2cfe106556) ![image](https://github.com/google/site-kit-wp/assets/94359491/5d56c327-7e3b-46b6-9ac3-9fda29a5b9d6) ![image](https://github.com/google/site-kit-wp/assets/94359491/23d9530d-a55a-404d-8f63-fb2debd2aa69) ![image](https://github.com/google/site-kit-wp/assets/94359491/a334d68b-4bab-4248-b37b-b73cc387eb25) https://github.com/google/site-kit-wp/assets/94359491/e3857366-dbe7-4de3-aeeb-f7dd48dcdfb6 https://github.com/google/site-kit-wp/assets/94359491/07cfd490-9703-4c0b-bc05-a813d8b36bda https://github.com/google/site-kit-wp/assets/94359491/31146162-e97f-452e-a28b-e520dea2783c