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.

Sign up for GitHub

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

Jump to bottom

PMax Assets: Add FAQs for the assets #1881

Merged
merged 4 commits into from
Feb 17, 2023
Merged

PMax Assets: Add FAQs for the assets #1881

merged 4 commits into from
Feb 17, 2023

Conversation

eason9487
Copy link
Member

@eason9487 eason9487 commented Feb 13, 2023 •
edited
Loading

Changes proposed in this Pull Request:

This PR implements a part of 📌 Add step 2 to the campaign management page in #1787.

  • Add FAQs

It also includes a part of #1833 - Gradually externalize @wordpress/components.

  • Fix the toggling button layout in the Panel along with the externalization for Panel, PanelBody, and PanelRow.
    2023-02-13 16 11 12

Screenshots:

📷 FAQs for the assets

image

📷 Externalization for Panel, PanelBody, and PanelRow

  • FAQs for Google Ads
    image
  • FAQs for Get Started
    2023-02-13 13 25 23
  • FAQs for accounts setup
    image
  • FAQs for Pre-launch checklist
    2023-02-13 13 32 21

Detailed test instructions:

  1. Check if the FAQs on both steps of the campaign creation and editing pages work properly.
  2. Check if the FAQs and the pre-launch checklist in the onboarding flow still work well.
  3. Check if the FAQs on the Get Started are still the same.

Changelog entry

Add - FAQs for the assets.
Dev - Externalize Panel, PanelBody, and PanelRow.

@eason9487 eason9487 self-assigned this Feb 13, 2023
@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 Feb 13, 2023
@eason9487 eason9487 mentioned this pull request Feb 13, 2023
Closed
34 tasks
@eason9487 eason9487 force-pushed the add/1787-assets-faqs branch from 10f6026 to 78769dd Compare February 17, 2023 05:38
@eason9487 eason9487 requested a review from a team February 17, 2023 05:44
@eason9487 eason9487 marked this pull request as ready for review February 17, 2023 05:44
puntope
puntope reviewed Feb 17, 2023
View reviewed changes
js/src/components/paid-ads/asset-group/faqs-section.js Outdated
/**
* Renders a toggleable FAQs section about the campaign assets of the Google Ads.
*
* @fires gla_faq with `{ context: 'campaign-management', id: 'what-will-my-ads-look-like', action: 'expand' }`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It not correct to merge both events and do something like.

@fires gla_faq with { context: 'campaign-management', id: 'what-will-my-ads-look-like', action: 'expand|collapse'}.

Not sure where but I think we used that way before.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an interesting question. I haven't noticed before but only copied them from the other place and changed the context and id.

After viewing the src/Tracking/README.md, yes, there are a few @fires tags that use | for indicating the possible values. In addition, the @event tag of gla_faq already stated the possible values of action, so I guess it should be fine to merge them into a single one by using | or even omit the action in the @fires description.

2023-02-17 18 15 54

Copy link
Member Author

Choose a reason for hiding this comment

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

Merged in 72353d6.

@puntope
Copy link
Contributor

puntope commented Feb 17, 2023

@eason9487 Where ca I see the source for the faqs? IS there anywhere in figma or???

@eason9487
Copy link
Member Author

Hi, @puntope

@eason9487 Where ca I see the source for the faqs? IS there anywhere in figma or???

Please refer to the Figma link of the i2 final version in the Links section of #1787.

@puntope
Copy link
Contributor

puntope commented Feb 17, 2023

Hi, @puntope

@eason9487 Where ca I see the source for the faqs? IS there anywhere in figma or???

Please refer to the Figma link of the i2 final version in the Links section of #1787.

Founded. Thanks!

puntope
puntope approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@puntope puntope left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM ✅

  • Faqs texts appear in the page and are equal than Figma reqs
  • All Panel related components externalised and working
  • Code looking good. (One comment about merging events with same params, but minor)

@eason9487 eason9487 merged commit b6c6e78 into feature/pmax-assets Feb 17, 2023
@eason9487 eason9487 deleted the add/1787-assets-faqs branch February 17, 2023 11:04
@eason9487 eason9487 mentioned this pull request Mar 3, 2023
Closed
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puntope puntope puntope approved these changes

Assignees

@eason9487 eason9487

Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eason9487 @puntope