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

2535: Consolidate Ad Creation #2623

Open
wants to merge 60 commits into
base: feature/2459-campaign-creation-flow
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
b207569
Move PaidAdsFeaturesSection and PaidAdsSetupSections components.
asvinb Aug 29, 2024
f65fbee
Save Work in progress.
asvinb Sep 2, 2024
1b8abc9
Consolidate AdsCampaign component.
asvinb Sep 3, 2024
3f8ac23
Revert local test changes.
asvinb Sep 3, 2024
101412b
Remove comments.
asvinb Sep 3, 2024
620b183
Clean up comments.
asvinb Sep 3, 2024
84879b8
Remove unused imports.
asvinb Sep 3, 2024
a3b50fe
Editing a campaign working.
asvinb Sep 3, 2024
a9b7f13
Fix condition to render paid ads sections.
asvinb Sep 3, 2024
c76a128
Address linting errors.
asvinb Sep 3, 2024
dcea1d6
Add headerDescription prop.
asvinb Sep 3, 2024
522e635
Remove unused comment.
asvinb Sep 3, 2024
388fddb
Remove onClick prop (?)
asvinb Sep 9, 2024
8ddde21
Show banner in create mode only.
asvinb Sep 9, 2024
ff7f281
Remove unused event prop
asvinb Sep 9, 2024
5200727
Remove unecessary isCreation prop.
asvinb Sep 9, 2024
d93bc1d
Add countryCodes field.
asvinb Sep 9, 2024
8e95434
Remove country code from edit screen.
asvinb Sep 10, 2024
2fa2bdb
Merge branch 'feature/2459-campaign-creation-flow' into update/2535-c…
asvinb Sep 23, 2024
6dd25e8
Add modal changes.
asvinb Sep 25, 2024
2f91ae8
Show paid ads sections by default.
asvinb Sep 25, 2024
3418caa
Simplify forms more.
asvinb Sep 25, 2024
7f25ff7
Move setup paid ads component.
asvinb Sep 25, 2024
7db2664
Remove unused braces.
asvinb Sep 25, 2024
892c016
Revert debugging changes.
asvinb Sep 25, 2024
9ba6eed
Fix tests for setup paid ads.
asvinb Sep 25, 2024
d7d5c6a
Remove unused args.
asvinb Sep 25, 2024
9c576b0
Fix hook return value.
asvinb Sep 25, 2024
c790719
Revert change.
asvinb Sep 25, 2024
35fed8a
Revert change.
asvinb Sep 25, 2024
fbce5c6
Set up events correctly.
asvinb Sep 25, 2024
814e353
Updated JSDocs.
asvinb Sep 25, 2024
17a869d
Remove unused files.
asvinb Sep 25, 2024
ca3dc21
Check for approval notice.
asvinb Sep 25, 2024
d4b6a8d
Update condition to show campaign preview card.
asvinb Sep 25, 2024
38d255a
Use CampaignAssetsForm within SetupPaidAds.
asvinb Sep 27, 2024
5343963
Do not load amount from local storage all the time except during onbo…
asvinb Sep 27, 2024
face49e
Merge branch 'feature/2458-streamline-onboarding' into update/2535-co…
asvinb Sep 27, 2024
b3e3b2a
Fix merge conflict for AdsCampaign.
asvinb Sep 27, 2024
d4c376d
Fix merge conflict for audienceSection test.
asvinb Sep 27, 2024
7cd8c5b
Delete unused file.
asvinb Sep 27, 2024
5d727ba
fix linting issues.
asvinb Sep 27, 2024
ab1f837
Merge pull request #2627 from woocommerce/update/2535-consolidate-ad-…
joemcgill Sep 30, 2024
34c53d7
Merge branch 'feature/2459-campaign-creation-flow' into update/2535-c…
joemcgill Sep 30, 2024
f3d74f2
Add continueButton and skipButton props.
asvinb Oct 8, 2024
8258b28
Use continueButton props.
asvinb Oct 8, 2024
d74087c
Save WIP.
asvinb Oct 8, 2024
b405e9c
Add ContinueButton component.
asvinb Oct 9, 2024
94cbe5f
Fix unit tests.
asvinb Oct 9, 2024
dd44feb
Only show BillingCard component during onboarding or Ads setup flow.
asvinb Oct 9, 2024
95d6452
Remove unused files.
asvinb Oct 9, 2024
229b41c
Address CR feedback.
asvinb Oct 11, 2024
6ce4cd2
Remove PaidAdsSetupSections component.
asvinb Oct 11, 2024
c239114
Restore campaign property.
asvinb Oct 11, 2024
bb710b3
Merge branch 'feature/2459-campaign-creation-flow' into update/2535-c…
asvinb Oct 11, 2024
db509b4
Fix unit tests.
asvinb Oct 11, 2024
7c958ad
Remove unused imports.
asvinb Oct 11, 2024
5ee5437
Fix condition to disable buttton.
asvinb Oct 11, 2024
4da52c5
Update E2E tests.
asvinb Oct 11, 2024
25ed467
Remove unused imports.
asvinb Oct 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 0 additions & 43 deletions js/src/components/audience-country-select.js

This file was deleted.

129 changes: 70 additions & 59 deletions js/src/components/paid-ads/ads-campaign/ads-campaign.js
Copy link
Member

Choose a reason for hiding this comment

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

Now that the AdsCampaign component has become more flexible and simple to use, however, there are still a few factors that can be polished.

  • Render logic depending on a prop named trackingContext seems inappropriate.
  • isOnboardingFlow and trackingContext could be consolidated and lifted to a higher abstraction level.
  • PaidAdsSetupSections is now purer and contains only one section.
  • Even if there is no need to render a BillingCard when creating or editing campaign, a redundant API request will still be sent to get the billing status.

Suggestions to further simplify the logic and divide component responsibilities:

  • Remove isOnboardingFlow prop and rename trackingContext to context.
    • context accepts one more possible string value, such as 'setup-mc'. to indicate it's being used in onboarding
    • Declare render condition variables within the components, for example:
      const isOnboardingFlow = context === 'setup-mc';
      const showBillingCard = context === 'setup-mc' || context === 'setup-ads';
  • Remove PaidAdsSetupSections to reduce the complexity of a layer of components.
    • Move the API request of getting billing status and SpinnerCard back to the BillingCard component
    • Moe the rest to the AdsCampaign component and use the above render condition variables, for example:
      <BudgetSection
      	formProps={ formContext }
      	countryCodes={ /* value for this prop needs to consider another comment */ }
      >
      	{ showBillingCard && <BillingCard /> }
      	{ ! isOnboardingFlow && <CampaignPreviewCard /> }
      </BudgetSection>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestions @eason9487 . Makes sense. Updated!

Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,22 @@
/**
* Internal dependencies
*/
import { useAdaptiveFormContext } from '.~/components/adaptive-form';
import StepContent from '.~/components/stepper/step-content';
import StepContentHeader from '.~/components/stepper/step-content-header';
import StepContentFooter from '.~/components/stepper/step-content-footer';
import StepContentActions from '.~/components/stepper/step-content-actions';
import StepContentFooter from '.~/components/stepper/step-content-footer';
import AppDocumentationLink from '.~/components/app-documentation-link';
import { useAdaptiveFormContext } from '.~/components/adaptive-form';
import AudienceSection from '../audience-section';
import BudgetSection from '../budget-section';
import { CampaignPreviewCard } from '../campaign-preview';
import PaidAdsFaqsPanel from '../faqs-panel';
import PaidAdsFaqsPanel from './faqs-panel';
import PaidAdsFeaturesSection from './paid-ads-features-section';
import CampaignPreviewCard from '.~/components/paid-ads/campaign-preview/campaign-preview-card';
import BudgetSection from '.~/components/paid-ads/budget-section';
import BillingCard from '.~/components/paid-ads/billing-card';
import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalCountryCodes';

/**
* @typedef {import('.~/components/adaptive-form/adaptive-form-context').AdaptiveFormContext} AdaptiveFormContext
*/

/**
* @typedef {import('.~/data/actions').Campaign} Campaign
Expand All @@ -30,81 +36,86 @@
*
* @fires gla_documentation_link_click with `{ context: 'create-ads' | 'edit-ads' | 'setup-ads', link_id: 'see-what-ads-look-like', href: 'https://support.google.com/google-ads/answer/6275294' }`
* @param {Object} props React props.
* @param {Campaign} [props.campaign] Campaign data to be edited. If not provided, this component will show campaign creation UI.
* @param {JSX.Element|Function} props.continueButton Continue button component.
* @param {'create-ads'|'edit-ads'|'setup-ads'} props.trackingContext A context indicating which page this component is used on. This will be the value of `context` in the track event properties.
* @param {Campaign} [props.campaign] Campaign data to be edited. The displayCountries property will be used to fetch budget recommendation data.
* @param {string} props.headerTitle The title of the step.
* @param {'create-ads'|'edit-ads'|'setup-ads'|'setup-mc'} props.context A context indicating which page this component is used on. This will be the value of `context` in the track event properties.
* @param {(AdaptiveFormContext) => JSX.Element|JSX.Element} [props.skipButton] A React element or function to render the "Skip" button. If a function is passed, it receives the form context and returns the button element.
* @param {(AdaptiveFormContext) => JSX.Element|JSX.Element} [props.continueButton] A React element or function to render the "Continue" button. If a function is passed, it receives the form context and returns the button element.
*/
export default function AdsCampaign( {
campaign,
headerTitle,
context,
skipButton,
continueButton,
trackingContext,
} ) {
const isCreation = ! campaign;
const formContext = useAdaptiveFormContext();
const { data: countryCodes } = useTargetAudienceFinalCountryCodes();
Copy link
Member

Choose a reason for hiding this comment

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

This countryCodes may lead to incorrect budget recommendation text and amount when editing an existing campaign having different target audience countries than the current ones in the free listings setup.

For example, given a campaign was created with one country code but is being edited after the free listings setup has been updated to have two country codes, the recommendation text will be incorrect:

  • Expected: Most merchants targeting Taiwan set a daily budget of 12 USD
  • Received: Most merchants targeting similar countries set a daily budget of 17 USD
Kapture.2024-10-10.at.15.46.15.mp4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const isOnboardingFlow = context === 'setup-mc';

Check warning on line 54 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L53-L54

Added lines #L53 - L54 were not covered by tests
const showCampaignPreviewCard =
context === 'setup-ads' ||
context === 'create-ads' ||
context === 'edit-ads';

Check warning on line 58 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L57-L58

Added lines #L57 - L58 were not covered by tests
// only show the billing card during onboarding or setup Ads flow.
// For creating/editing a campaign, we assume billing is already set up.
const showBillingCard = context === 'setup-mc' || context === 'setup-ads';

let description = createInterpolateElement(

Check warning on line 63 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L63

Added line #L63 was not covered by tests
__(
'Paid Performance Max campaigns are automatically optimized for you by Google. <link>See what your ads will look like.</link>',
'google-listings-and-ads'
),
{
link: (
<AppDocumentationLink
context={ context }
linkId="see-what-ads-look-like"
href="https://support.google.com/google-ads/answer/6275294"
/>
),
}
);

const disabledBudgetSection = ! formContext.values.countryCodes.length;
const helperText = isCreation
? __(
'You can only choose from countries you’ve selected during product listings configuration.',
'google-listings-and-ads'
)
: __(
'Once a campaign has been created, you cannot change the target country(s).',
'google-listings-and-ads'
);
if ( isOnboardingFlow ) {
description = __(

Check warning on line 80 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L80

Added line #L80 was not covered by tests
'You’re ready to set up a Performance Max campaign to drive more sales with ads. Your products will be included in the campaign after they’re approved.',
'google-listings-and-ads'
);
}

return (
<StepContent>
<StepContentHeader
title={
isCreation
? __(
'Create your paid campaign',
'google-listings-and-ads'
)
: __(
'Edit your paid campaign',
'google-listings-and-ads'
)
}
description={ createInterpolateElement(
__(
'Paid Performance Max campaigns are automatically optimized for you by Google. <link>See what your ads will look like.</link>',
'google-listings-and-ads'
),
{
link: (
<AppDocumentationLink
context={ trackingContext }
linkId="see-what-ads-look-like"
href="https://support.google.com/google-ads/answer/6275294"
/>
),
}
) }
/>
<AudienceSection
disabled={ ! isCreation }
multiple={ isCreation || campaign.allowMultiple }
countrySelectHelperText={ helperText }
formProps={ formContext }
title={ headerTitle }
description={ description }
/>

{ isOnboardingFlow && <PaidAdsFeaturesSection /> }

Check warning on line 93 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L93

Added line #L93 was not covered by tests

<BudgetSection
formProps={ formContext }
disabled={ disabledBudgetSection }
countryCodes={ formContext.values.countryCodes }
countryCodes={
context === 'edit-ads'
? campaign?.displayCountries
: countryCodes

Check warning on line 100 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L99-L100

Added lines #L99 - L100 were not covered by tests
}
>
<CampaignPreviewCard />
{ showBillingCard && <BillingCard /> }

Check warning on line 103 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L103

Added line #L103 was not covered by tests

{ showCampaignPreviewCard && <CampaignPreviewCard /> }

Check warning on line 105 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L105

Added line #L105 was not covered by tests
</BudgetSection>

<StepContentFooter>
<StepContentActions>
{ typeof skipButton === 'function'
? skipButton( formContext )
: skipButton }

Check warning on line 112 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L111-L112

Added lines #L111 - L112 were not covered by tests

{ typeof continueButton === 'function'
? continueButton( {
formProps: formContext,
} )
? continueButton( formContext )

Check warning on line 115 in js/src/components/paid-ads/ads-campaign/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign/ads-campaign.js#L115

Added line #L115 was not covered by tests
: continueButton }
</StepContentActions>

<PaidAdsFaqsPanel />
</StepContentFooter>
</StepContent>
Expand Down
84 changes: 0 additions & 84 deletions js/src/components/paid-ads/audience-section.js

This file was deleted.

15 changes: 0 additions & 15 deletions js/src/components/paid-ads/audience-section.scss

This file was deleted.

Loading