Update Channel Visibility meta box UI to match the latest designs#3262
Update Channel Visibility meta box UI to match the latest designs#3262ankitrox wants to merge 26 commits intofeature/in-product-placementsfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/in-product-placements #3262 +/- ##
=================================================================
+ Coverage 66.3% 66.7% +0.5%
- Complexity 5118 5119 +1
=================================================================
Files 507 868 +361
Lines 20390 26249 +5859
Branches 0 1477 +1477
=================================================================
+ Hits 13515 17518 +4003
- Misses 6875 8558 +1683
- Partials 0 173 +173
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
asvinb
left a comment
There was a problem hiding this comment.
@AlejandroPerezMartin Can you kindly check my comment please? Also, if you can merge the latest from feature/in-product-placements and merge any conflicts please.
js/src/meta-boxes/channel-visibility/google-ads-promo-setup-completed.js
Outdated
Show resolved
Hide resolved
js/src/meta-boxes/channel-visibility/google-ads-promo-setup-completed.js
Outdated
Show resolved
Hide resolved
…king-events GOOWOO-461 Add channel visibility box tracking
…/google-listings-and-ads into update/GOOWOO-450-metabox-ui
…e/google-listings-and-ads into update/GOOWOO-450-metabox-ui
asvinb
left a comment
There was a problem hiding this comment.
@AlejandroPerezMartin I left some comments in the PR. Can you kindly check them out please? Also there are a few things missing:
@firesJS Docs for events- updating the tracking doc via
npm run doc:tracking - When I select a value from the select, hits the update button, the value is not persisted upon page refresh
- Update the test instructions please.
|
|
||
| const GoogleAdsPromoSetupCompleted = () => { | ||
| const [ channelVisibility, setChannelVisibility ] = useState( | ||
| product_is_visible ? channel_visibility : 'dont-sync-and-show' |
There was a problem hiding this comment.
@AlejandroPerezMartin Why this condition is tied to whether the product is visible? 🤔 ?
There was a problem hiding this comment.
There was a problem hiding this comment.
@AlejandroPerezMartin Any chance of not hard coding the 'dont-sync-and-show' value?
There was a problem hiding this comment.
don't think so, same logic and issues was in the php version, there's no way to know that unless we get rid of that logic
js/src/meta-boxes/channel-visibility/google-ads-promo-setup-completed.js
Outdated
Show resolved
Hide resolved
js/src/meta-boxes/channel-visibility/google-ads-promo-setup-completed.js
Outdated
Show resolved
Hide resolved
| * Google Ads Promo "Get started" button is clicked. | ||
| * | ||
| * @event gla_google_ads_promo_create_campaign_click | ||
| * @property {string} context Context of the Google Ads Promo. |
There was a problem hiding this comment.
@AlejandroPerezMartin Let's add the context name here since we already know the value.
There was a problem hiding this comment.
it's already present a few lines below in the event definition https://github.com/woocommerce/google-listings-and-ads/pull/3262/changes/BASE..1a81b54f8b1cfb2adfa8a2305763322231e3683d#diff-ec3e9904fae879de706865a6905899dfeeea6ecd2f92fa94dd221ad081c3e546R24
| /** | ||
| * Get Started CTA component. | ||
| * | ||
| * @fires gla_google_ads_promo_create_campaign_click with `{ context: CHANNEL_VISIBILITY_CONTEXT, href: getGetStartedUrl() }`. |
There was a problem hiding this comment.
@AlejandroPerezMartin Let's use the values here instead of the CHANNEL_VISIBILITY_CONTEXT and the getGetStartedUrl() function. Also there's a typo in the function name.
| /** | ||
| * Google Ads Promo CTA component. | ||
| * | ||
| * @fires gla_google_ads_promo_dismiss_click with `{ context: CHANNEL_VISIBILITY_CONTEXT }`. |
|
|
||
| const GoogleAdsPromoSetupCompleted = () => { | ||
| const [ channelVisibility, setChannelVisibility ] = useState( | ||
| product_is_visible ? channel_visibility : 'dont-sync-and-show' |
There was a problem hiding this comment.
@AlejandroPerezMartin Any chance of not hard coding the 'dont-sync-and-show' value?
| * | ||
| * @fires gla_google_ads_promo_shown with `{ context: CHANNEL_VISIBILITY_CONTEXT }`. | ||
| * | ||
| * @return {JSX.Element|null} The Google Ads Promo component or null. |
There was a problem hiding this comment.
@AlejandroPerezMartin Is the comment up to date since I don't see the component returning null 🤔
| import { glaData } from '~/constants'; | ||
|
|
||
| const GoogleAdsPromo = lazy( () => | ||
| import( /* webpackChunkName: "google-ads-promo" */ './google-ads-promo' ) |
There was a problem hiding this comment.
| import( /* webpackChunkName: "google-ads-promo" */ './google-ads-promo' ) | |
| import( /* webpackChunkName: "channel-visibility-google-ads-promo" */ './google-ads-promo' ) |
Just so that there's no conflict with the order attribution one.
| } = {}, | ||
| } = glaData || {}; | ||
|
|
||
| const GoogleAdsPromoSetupCompleted = () => { |
There was a problem hiding this comment.
@AlejandroPerezMartin Let's add some JSDocs explaining when this component is rendered.
| * | ||
| * @return {JSX.Element} The Google Ads Promo CTA component. | ||
| */ | ||
| const GoogleAdsPromoCTA = ( { onDismiss } ) => { |
There was a problem hiding this comment.
@AlejandroPerezMartin I feel having GoogleAds in the component name is redundant. Can we just name it PromoCTA or PromoActions? Same thing applies to GoogleAdsPromoSetupCompleted component. I feel the GoogleAds prefix is unecessary.
| } = {}, | ||
| } = glaData || {}; | ||
|
|
||
| const GoogleAdsPromoSetupCompleted = () => { |
There was a problem hiding this comment.
@AlejandroPerezMartin Also maybe we should rename the component to ChannelVisibilitySettings.
Changes proposed in this Pull Request:
Closes: https://linear.app/a8c/issue/GOOWOO-450/update-channel-visibility-meta-box-ui-to-match-the-latest-designs
Screenshots:
Detailed test instructions:
gla_google_ads_promo_shownchannel-visibility-meta-boxgla_google_ads_promo_create_campaign_clickchannel-visibility-meta-boxgla_google_ads_promo_get_started_clickchannel-visibility-meta-boxchannel-visibility-meta-boxAdditional details:
Changelog entry