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

Free Listings + Paid Ads: Add ad previews to the post-onboarding ads setup flow #1721

Merged

Conversation

eason9487
Copy link
Member

Changes proposed in this Pull Request:

This PR implements an item of the 📌 Post-onboarding setup flow in #1610.

  • Add ad previews to the post-onboarding ads setup flow.

Screenshots:

Kapture.2022-10-07.at.18.18.08.mp4

Detailed test instructions:

  1. Go to step 2 of the post-onboarding ads setup flow. /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsetup-ads
  2. Click on the next and previous buttons to see if the ad previews work as expected.

Changelog entry

Add - Ad previews in the post-onboarding ads setup flow.

@eason9487 eason9487 requested a review from a team October 7, 2022 10:23
@eason9487 eason9487 self-assigned this Oct 7, 2022
@github-actions github-actions bot added changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement. labels Oct 7, 2022
@@ -26,6 +32,9 @@ import './campaign-preview.scss';

/**
* @typedef { import("./index.js").AdPreviewData } AdPreviewData
*
* @typedef {Object} CampaignPreviewHandler
* @property {(step: number) => void} moveBy Move the currently displayed preview ad by how many steps, e.g. 1 for moving to the next and -1 for the previous.
Copy link
Contributor

@puntope puntope Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💅 I guess steps should be more accurate than step since ads far as I know it could be more than 1 step movement.

Besides that, since we don't use movements of more than one step. I guess it could be refactored as

move(backwards : bool = false) and move the preview one step backwards if backwards is true.

Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM

I tested it locally and I can see the arrows that allows to move the preview campaign

I left some comments regarding code. But Approved in advance since they are. 💅

@ianlin ianlin merged commit 24498c8 into feature/1610-streamlined-onboarding Oct 12, 2022
@ianlin ianlin deleted the add/1610-post-onboarding-ad-preview branch October 12, 2022 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants