-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: feature/2459-campaign-creation-flow
Are you sure you want to change the base?
Changes from 44 commits
b207569
f65fbee
1b8abc9
3f8ac23
101412b
620b183
84879b8
a3b50fe
a9b7f13
c76a128
dcea1d6
522e635
388fddb
8ddde21
ff7f281
5200727
d93bc1d
8e95434
2fa2bdb
6dd25e8
2f91ae8
3418caa
7f25ff7
7db2664
892c016
9ba6eed
d7d5c6a
9c576b0
c790719
35fed8a
fbce5c6
814e353
17a869d
ca3dc21
d4b6a8d
38d255a
5343963
face49e
b3e3b2a
d4c376d
7cd8c5b
5d727ba
ab1f837
34c53d7
f3d74f2
8258b28
d74087c
b405e9c
94cbe5f
dd44feb
95d6452
229b41c
6ce4cd2
c239114
bb710b3
db509b4
7c958ad
5ee5437
4da52c5
25ed467
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,189 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
import { | ||
createInterpolateElement, | ||
useState, | ||
useEffect, | ||
} from '@wordpress/element'; | ||
import { noop } from 'lodash'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { useAdaptiveFormContext } from '.~/components/adaptive-form'; | ||
import StepContent from '.~/components/stepper/step-content'; | ||
import StepContentHeader from '.~/components/stepper/step-content-header'; | ||
import StepContentActions from '.~/components/stepper/step-content-actions'; | ||
import StepContentFooter from '.~/components/stepper/step-content-footer'; | ||
import AppDocumentationLink from '.~/components/app-documentation-link'; | ||
import AppButton from '.~/components/app-button'; | ||
import PaidAdsFaqsPanel from './faqs-panel'; | ||
import PaidAdsFeaturesSection from './paid-ads-features-section'; | ||
import PaidAdsSetupSections from './paid-ads-setup-sections'; | ||
import SkipButton from './skip-button'; | ||
import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalCountryCodes'; | ||
import { ACTION_SKIP, ACTION_COMPLETE } from './constants'; | ||
|
||
/** | ||
* @typedef {import('.~/data/actions').Campaign} Campaign | ||
*/ | ||
|
||
/** | ||
* Clicking on the "Complete setup" button to complete the onboarding flow with paid ads. | ||
* | ||
* @event gla_onboarding_complete_with_paid_ads_button_click | ||
* @property {number} budget The budget for the campaign | ||
* @property {string} audiences The targeted audiences for the campaign | ||
*/ | ||
|
||
/** | ||
* Renders the container of the form content for campaign management. | ||
* | ||
* Please note that this component relies on an CampaignAssetsForm's context and custom adapter, | ||
* so it expects a `CampaignAssetsForm` to existing in its parents. | ||
* | ||
* @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' }` | ||
* @fires gla_onboarding_complete_with_paid_ads_button_click | ||
* @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 {string} props.headerTitle The title of the step. | ||
* @param {() => void} props.onContinue Callback called once continue button is clicked. | ||
* @param {() => void} [props.onSkip] Callback called once skip button is clicked. | ||
* @param {boolean} [props.hasError=false] Whether there's an error to reset the completing state. | ||
* @param {boolean} [props.isOnboardingFlow=false] Whether this component is used in onboarding flow. | ||
* @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. | ||
*/ | ||
export default function AdsCampaign( { | ||
campaign, | ||
headerTitle, | ||
onContinue, | ||
onSkip = noop, | ||
hasError = false, | ||
isOnboardingFlow = false, | ||
trackingContext, | ||
} ) { | ||
const formContext = useAdaptiveFormContext(); | ||
const { data: countryCodes } = useTargetAudienceFinalCountryCodes(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This 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:
Kapture.2024-10-10.at.15.46.15.mp4There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eason9487 This should be fixed now. We are now passing the |
||
const { isValidForm, setValue } = formContext; | ||
const [ completing, setCompleting ] = useState( null ); | ||
const [ paidAds, setPaidAds ] = useState( {} ); | ||
|
||
useEffect( () => { | ||
if ( hasError ) { | ||
setCompleting( null ); | ||
} | ||
}, [ hasError ] ); | ||
|
||
const handleOnStatesReceived = ( paidAdsValues ) => { | ||
setPaidAds( paidAdsValues ); | ||
|
||
const { amount } = paidAdsValues; | ||
setValue( 'amount', amount ); | ||
}; | ||
|
||
const handleSkipCreateAds = () => { | ||
setCompleting( ACTION_SKIP ); | ||
|
||
onSkip( paidAds ); | ||
}; | ||
|
||
const handleCompleteClick = ( event ) => { | ||
setCompleting( event.target.dataset.action ); | ||
|
||
onContinue( paidAds ); | ||
}; | ||
|
||
// The status check of Google Ads account connection is included in `paidAds.isReady`, | ||
// because when there is no connected account, it will disable the budget section and set the `amount` to `undefined`. | ||
const disabledComplete = | ||
completing === ACTION_SKIP || ! paidAds.isReady || ! isValidForm; | ||
|
||
let continueButtonProps = { | ||
text: __( 'Continue', 'google-listings-and-ads' ), | ||
}; | ||
|
||
if ( isOnboardingFlow ) { | ||
continueButtonProps = { | ||
'data-action': ACTION_COMPLETE, | ||
text: __( 'Complete setup', 'google-listings-and-ads' ), | ||
eventName: 'gla_onboarding_complete_with_paid_ads_button_click', | ||
eventProps: { | ||
budget: paidAds.amount, | ||
audiences: countryCodes?.join( ',' ), | ||
}, | ||
}; | ||
} | ||
|
||
let 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" | ||
/> | ||
), | ||
} | ||
); | ||
|
||
if ( isOnboardingFlow ) { | ||
description = __( | ||
'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={ headerTitle } | ||
description={ description } | ||
/> | ||
|
||
{ isOnboardingFlow && <PaidAdsFeaturesSection /> } | ||
|
||
<PaidAdsSetupSections | ||
onStatesReceived={ handleOnStatesReceived } | ||
campaign={ campaign } | ||
countryCodes={ countryCodes } | ||
loadCampaignFromClientSession={ isOnboardingFlow } | ||
showCampaignPreviewCard={ | ||
trackingContext === 'setup-ads' || | ||
trackingContext === 'create-ads' | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the first acceptance criteria:
I would like to confirm what the scope of the “same form” is, after all, they are currently presented differently in the campaign preview. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "same form" means that we will use the same component for both forms so that the requirements of each input section are consistent. The main difference is that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation. I would suggest rephrasing it a little bit to avoid having “When a user …, they will see …” wording that confuses its goal of just having to share components in development. Or, clarify the goal is to have the same UI as before the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before this PR, the preview card is shown as well when editing campaign. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated! |
||
/> | ||
eason9487 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
<StepContentFooter> | ||
<StepContentActions> | ||
{ isOnboardingFlow && ( | ||
<SkipButton | ||
text={ __( | ||
'Skip paid ads creation', | ||
'google-listings-and-ads' | ||
) } | ||
onSkipCreatePaidAds={ handleSkipCreateAds } | ||
completing={ completing } | ||
paidAds={ paidAds } | ||
/> | ||
) } | ||
eason9487 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
<AppButton | ||
isPrimary | ||
disabled={ disabledComplete } | ||
onClick={ handleCompleteClick } | ||
loading={ completing === ACTION_COMPLETE } | ||
{ ...continueButtonProps } | ||
eason9487 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/> | ||
</StepContentActions> | ||
|
||
<PaidAdsFaqsPanel /> | ||
</StepContentFooter> | ||
</StepContent> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { default } from './ads-campaign'; |
There was a problem hiding this comment.
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.trackingContext
seems inappropriate.isOnboardingFlow
andtrackingContext
could be consolidated and lifted to a higher abstraction level.PaidAdsSetupSections
is now purer and contains only one section.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:
isOnboardingFlow
prop and renametrackingContext
tocontext
.context
accepts one more possible string value, such as'setup-mc'
. to indicate it's being used in onboardingPaidAdsSetupSections
to reduce the complexity of a layer of components.SpinnerCard
back to theBillingCard
componentAdsCampaign
component and use the above render condition variables, for example:There was a problem hiding this comment.
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!