Skip to content
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

Create Ads PAX Setup Success Notification Banner #8661

Closed
6 of 11 tasks
10upsimon opened this issue May 6, 2024 · 7 comments
Closed
6 of 11 tasks

Create Ads PAX Setup Success Notification Banner #8661

10upsimon opened this issue May 6, 2024 · 7 comments
Labels
javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P1 Medium priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented May 6, 2024

Feature Description

When a PAX setup flow for Ads has been completely successfully, the user should be redirected to the main Site Kit dashboard and a campaign setup success notification banner should be displayed. This banner should contain a CTA that takes the user to the applicable reporting widget on the dashboard, and also be dismissible.

The Figma design can be viewed here. Following is a screenshot of the success banner design:

image.png


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

Acceptance criteria

  • Upon successfully completing the Ads PAX onboarding and creating a campaign, the user should be redirected to the main Site Kit dashboard and be presented with a success banner notification as per the Figma design.
  • This banner should contain two CTA's:
    • A main/primary CTA button labelled "Show Me" which should direct the user to the applicable Ads reporting widget on the main Site Kit dashboard and dismiss the notification.
    • A secondary CTA labelled "Got it" which should only dismiss the success banner notification.
  • Upon dismissing the notification, said banner should not be displayed to the user again

Implementation Brief

Subtle Notification Component

  • Create a new component assets/js/modules/ads/components/notifications/PAXSetupSuccessSubtleNotification.js based on the existing assets/js/components/notifications/SetupSuccessSubtleNotification.js component updating the copy based on the FIGMA designs.

  • While working on this ticket, move the assets/js/components/notifications/SetupSuccessSubtleNotification.js component to assets/js/modules/ads/components/notifications/SetupSuccessSubtleNotification.js as this notification is currently specific to the Ads module.

    • Export a new constant PAX_SETUP_SUCCESS_NOTIFICATION within assets/js/modules/ads/components/notifications/PAXSetupSuccessSubtleNotification.js with the value pax_setup_success_notification which will be used to reference this notification in this component and in the redirect from the PAX service function.
    • This component should use useQueryArg to check if the notification does not equal PAX_SETUP_SUCCESS_NOTIFICATION and return null in this case.
    • Implement the "Got it" CTA just like it is in assets/js/components/notifications/SetupSuccessSubtleNotification.js, however we don't need to update the slug query arg, only remove the notification query arg when the onDismiss function is called.
    • Implement the "Show me" CTA as a primary Button component, adding relevant CSS to confirm it matches the FIGMA designs, and creating an onClick function called onShowMe:
      • Within the onShowMe function use the getContextScrollTop util to get the y offset of the PAX by passing the .googlesitekit-pax-embedded-app selector in the first argument and the current breakpoint in the second argument.
      • Use global.scrollTo, with top set to the value returned by getContextScrollTop, with behaviour set to smooth.
  • Import and render the new PAXSetupSuccessSubtleNotification component within assets/js/components/notifications/SubtleNotifications.js

PAX Setup Complete Redirect

  • Update the onSetupComplete function in assets/js/modules/ads/components/setup/SetupMainPAX.js, passing a custom redirectURL to the finishSetup function.
    • The redirect URL can be retrieved using getAdminURL( 'googlesitekit-dashboard', { notification: PAX_SETUP_SUCCESS_NOTIFICATION } ).

Test Coverage

  • Create stories for PAXSetupSuccessSubtleNotification and SetupSuccessSubtleNotification and add them to VRTs.

QA Brief

  • Setup SIte Kit with adsPax feature flag enabled
  • Setup the Ads module using create an account button, and finalize the flow in the PAX app
  • After completing the steps, you will be redirected to the dashboard - verify that success subtle notification is showing and it matches the figma design
  • Clicking the "Got it" CTA button/link should dismiss the notification banner. It should not appear again on page refresh after this.
  • Clicking on the "Show me" CTA button/link should dismiss the notification banner and scroll the page down to the ads section of the page. It should not appear again on page refresh after this.

Changelog entry

  • Add PAX notification banner when PAX setup is completed successfully.
@10upsimon 10upsimon added javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P1 Medium priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature labels May 6, 2024
@tofumatt tofumatt self-assigned this May 6, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented May 6, 2024

ACs are good, moving to IB 👍🏻

@tofumatt tofumatt removed their assignment May 6, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler May 7, 2024
@aaemnnosttv aaemnnosttv self-assigned this May 7, 2024
@aaemnnosttv
Copy link
Collaborator

@benbowler IB looks good overall, just a few points to revise I think.

  • Update assets/js/modules/ads/pax/services.js, adding a new service function

This is being done in #8560, so it's out of scope here.

The change to which notification to trigger in the URL should be done in the SetupMainPAX via finishSetup.

As an aside, the SetupSuccessSubtleNotification (+ PAX variant added here) should be relocated within the Ads module structure rather than in our top-level components. Otherwise it seems like a generic component, but is Ads-specific. This is somewhat unrelated, but small enough to include here 👍

@aaemnnosttv aaemnnosttv assigned benbowler and unassigned aaemnnosttv May 13, 2024
@benbowler benbowler assigned aaemnnosttv and unassigned benbowler May 14, 2024
@benbowler
Copy link
Collaborator

Thank @aaemnnosttv, I've addressed your comments.

@aaemnnosttv
Copy link
Collaborator

Thanks @benbowler – one more thing

  • Create stories for PAXSetupSuccessSubtleNotification and SetupSuccessSubtleNotification and add them to VRTs.

Stories are great, but we don't need to add VRTs for all components. Unless there's something specific about the styling for this component compared to other subtle notifications, we can assume that the rest will be the same. We should have one (group?) of VRTs for subtle notifications that covers them all, since they all really leverage the same markup and styles.

Ideally I think we'd have a base SubtleNotification component that each uses internally (rather than duplicating the markup within). We don't need to go this far as part of this issue though, so it's probably worth doing this in a separate issue.

For now, I've just removed the bit about adding VRTs, otherwise this is G2G!

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment May 14, 2024
@benbowler
Copy link
Collaborator

@aaemnnosttv sounds good, yes it definitely makes sense to generalise the SubtleNotification in future.

@tofumatt
Copy link
Collaborator

@aaemnnosttv @benbowler I have it on my todo list to create issues for the subtle notification component to standardise it. The reason we didn't from the outset was I wanted a few instances in real-world use to extract the component from rather than prescriptively creating the component not knowing how we'd use it 🙂

But now that we have a few examples of it I think we can generalise it. I'll create that issue(s) soon, it's just been a bit lower priority 😅

@tofumatt tofumatt removed their assignment May 21, 2024
@zutigrm zutigrm assigned tofumatt and unassigned zutigrm May 22, 2024
@tofumatt tofumatt assigned zutigrm and unassigned tofumatt May 23, 2024
@zutigrm zutigrm assigned tofumatt and unassigned zutigrm May 24, 2024
@tofumatt tofumatt removed their assignment May 27, 2024
@mohitwp mohitwp self-assigned this May 28, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented May 29, 2024

QA Update ✅

  • Tested on dev environment.
  • Verified that if Ads Pax feature flag is enabled then Ads Pax Setup success notification banner is showing on main dashboard.
  • Verified functionality of 'Got it' and 'Show me' CTA. Both are working as per AC.
  • Verified notification banner design with the Figma.
  • Verified notification banner responsiveness.

image

'Show me' CTA

Recording.999.mp4

'Got it' CTA

Recording.1001.mp4

@mohitwp mohitwp removed their assignment May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P1 Medium priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants