Free Listings + Paid Ads: Add the paid ads previews to the boost product listings section #1650

eason9487 · 2022-08-24T11:31:03Z

Changes proposed in this Pull Request:

This PR implements the ad previews part of 📌 Boost product listings section in #1610.

Dynamically compositing the ad previews with product and shop data.
💡Please note that there is a very small white border in the fallback image of product cover. It's not a CSS style problem and I will check with the design team later.
- Fixed in 6fe6ba7.

Screenshots:

📷 Text ellipsis

📷 Simulation of using product cover from merchant's DB

📷 Simulation of using shop logo from merchant's website

📹 All ad previews and blur animation

Kapture.2022-08-24.at.17.17.08.mp4

Detailed test instructions:

Go to step 4 of the onboarding flow.
Check if the visual result of the ad previews in the "Boost product listings with paid ads" section is close to the design in Figma.
Change these values locally to see if the layouts of ad previews are still rendered properly.

…eter to be optional

…size since there is an unavoidable increase in this PR

…the image

puntope · 2022-08-29T13:05:28Z

.eslintrc.js

@@ -28,6 +28,7 @@ module.exports = {
 			'stylelint',
 			'@wordpress/stylelint-config',
 			'@pmmmwh/react-refresh-webpack-plugin',
+			'react-transition-group',


❓ Just curiosity. Not an issue. Why is needed to add this dependency here?

This PR uses the same react-transition-group dependency as the one @woocommerce/components depends on, so react-transition-group is not listed in this repo's package.json file. Therefore, this change is to let eslint know it's one of the core modules and to prevent from the eslint error import/no-extraneous-dependencies.

puntope · 2022-08-29T13:15:31Z

js/src/components/paid-ads/campaign-preview/components/placeholder.js

+ * @param {boolean} [props.thinnest] Whether to draw a thinnest placeholder.
+ * @param {boolean} [props.thinner] Whether to draw a thinner placeholder.
+ * @param {boolean} [props.thicker] Whether to draw a thicker placeholder.


💅 I see this just set the height of the placeholder??

Why not just type="thinnest|thinner|thicker" ? Or just do height={unit} and set up the height as we do with the width.

I forgot the original reason. I probably started with three different values of height and was inspired by the props of Button component (isPrimary, isSecondary, isLink, etc.). But when adding the color, it was not so ideal to use the same pattern, so I changed to using a single prop with different values.

And it's definitely better to have consistent props for this component. I changed them in c6908de.

P.S. I have double-checked the visual results again with Figma file when doing this change. And some prop values were different from their previous value in this commit since I found them are not the closest ones to the visual design. Not sure if Figma file was changed or if I set the wrong values in the beginning. 😅

puntope · 2022-08-29T13:18:52Z

js/src/components/paid-ads/campaign-preview/components/scaled-text.js

+ * @param {boolean} [props.smaller] Whether to draw a smaller text size.
+ * @param {boolean} [props.larger] Whether to draw a larger text size.


💅 Same as the case of thinner/ticker... Why not just set a type prop and add the class name modifier? Or do the inline style mdoification?

Changed in 631e8fe.

puntope · 2022-08-29T13:21:21Z

❓ Regarding google shopping logo (sorry somehow I don't know how to comment in the file)

Shall we check if that is still the Shopping logo we want? I just modified them recently with the new logo

puntope · 2022-08-29T13:22:59Z

js/src/components/paid-ads/campaign-preview/index.js

+ * Data for compositing ad preview mockups.
+ *
+ * @typedef {Object} AdPreviewData
+ * @property {string} title Price of the product.


❓ Should this be "Title of the product"?

Thanks for catching this. Fixed in 8e72981.

puntope · 2022-08-29T13:29:38Z

js/src/components/paid-ads/campaign-preview/campaign-preview.js

+	const { second, callCount, startCountdown } = useCountdown();
+
+	// TODO: Fetch required data from API.
+	const product = fallbackProduct;
+
+	const moveBy = useCallback( ( step ) => {
+		setIndex( ( currentIndex ) => {
+			return ( currentIndex + step + mockups.length ) % mockups.length;
+		} );
+	}, [] );
+
+	useEffect( () => {
+		if ( second === 0 ) {
+			if ( callCount > 0 ) {
+				moveBy( 1 );
+			}
+			startCountdown( 5 );
+		}
+	}, [ second, callCount, startCountdown, moveBy ] );


💅 This maybe can be wrapped in some kind of useTimingIndex

Wrapping this to another custom hook sometimes also increases the complexity. In this case, the CampaignPreview component doesn't have much code, therefore, I think it ok to stay with the current implementation.

puntope · 2022-08-29T13:38:28Z

💅 Maybe some adjustments needed for resolutions between 500-700px

…w/index.js Address #1650 (comment)

…orts. Address #1650 (comment)

…r Placeholder and adjust some values of them to align with visual design. Address #1650 (comment)

Address #1650 (comment)

eason9487 · 2022-08-30T05:16:43Z

@puntope, thanks for the review.

❓ Regarding google shopping logo (sorry somehow I don't know how to comment in the file)
Shall we check if that is still the Shopping logo we want? I just modified them recently with the new logo

I checked this with the design team, and it's confirmed that the logo is correct for this project. Because it's supposed to be a Google Shopping logo on the ad preview of Google Shopping.

💅 Maybe some adjustments needed for resolutions between 500-700px

Thanks for catching this. I added some CSS styles for this in 656aeb9.

I believe all comments were replied to or fixed. Could you help with another round of code reviews?

puntope

✅ LGTM

💅 One thing I'm missing here and in this feature are the tests

eason9487 added 11 commits August 24, 2022 18:18

Add new images for the mockups of ad previews

8735780

Add the container for ad previews

1bde4a9

Implement shared components for the mockups of ad previews

7e1d5a5

Implement the Google Shopping mockup of ad previews

ca99756

Implement the YouTube mockup of ad previews

88cfb40

Implement the Google Search mockup of ad previews

87ce3bc

Implement the Google Map mockup of ad previews

ced245d

Implement the Google Display Network mockup of ad previews

1aa32c6

Implement the Gmail mockup of ad previews

3b73e32

Move useCountdown.js to hooks directory and change the handle param…

f3cc043

…eter to be optional

Add ad previews to the paid ads features section

05779b9

eason9487 added the changelog: none Skip changelog entry for this PR label Aug 24, 2022

eason9487 requested a review from a team August 24, 2022 11:31

eason9487 self-assigned this Aug 24, 2022

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 Aug 24, 2022

Rebalance the bundlewatch's max size of index.css to 120% of the new …

3279a21

…size since there is an unavoidable increase in this PR

eason9487 removed type: enhancement The issue is a request for an enhancement. changelog: add A new feature, function, or functionality was added. labels Aug 24, 2022

eason9487 mentioned this pull request Aug 25, 2022

Streamlined Free Listings plus Paid Ads for the onboarding flow #1610

Closed

35 tasks

Update the sample product image file to remove the white border from …

6fe6ba7

…the image

puntope reviewed Aug 29, 2022

View reviewed changes

Base automatically changed from add/1610-paid-ads-features-section to feature/1610-streamlined-onboarding August 30, 2022 03:18

Fix a typo in the JSDoc of js/src/components/paid-ads/campaign-previe…

8e72981

…w/index.js Address #1650 (comment)

eason9487 added 3 commits August 30, 2022 12:13

Fix the broken layout of PaidAdsFeaturesSection for the smaller viewp…

656aeb9

…orts. Address #1650 (comment)

Merge thinnest, thinner, and thicker props as a single stroke prop fo…

c6908de

…r Placeholder and adjust some values of them to align with visual design. Address #1650 (comment)

Merge smaller and larger props as a single size prop for ScaledText.

631e8fe

Address #1650 (comment)

eason9487 requested a review from puntope August 30, 2022 05:19

puntope approved these changes Aug 30, 2022

View reviewed changes

eason9487 merged commit 722ca0a into feature/1610-streamlined-onboarding Aug 30, 2022

eason9487 deleted the add/1610-paid-ads-previews branch August 30, 2022 09:16

eason9487 added changelog: add A new feature, function, or functionality was added. and removed changelog: none Skip changelog entry for this PR labels Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Free Listings + Paid Ads: Add the paid ads previews to the boost product listings section #1650

Free Listings + Paid Ads: Add the paid ads previews to the boost product listings section #1650

eason9487 commented Aug 24, 2022 •

edited

Loading

puntope Aug 29, 2022

eason9487 Aug 30, 2022

puntope Aug 29, 2022

eason9487 Aug 30, 2022

puntope Aug 29, 2022

eason9487 Aug 30, 2022

puntope commented Aug 29, 2022 •

edited

Loading

puntope Aug 29, 2022

eason9487 Aug 30, 2022

puntope Aug 29, 2022

eason9487 Aug 30, 2022

puntope commented Aug 29, 2022 •

edited

Loading

eason9487 commented Aug 30, 2022

puntope left a comment

		* @param {boolean} [props.smaller] Whether to draw a smaller text size.
		* @param {boolean} [props.larger] Whether to draw a larger text size.

Free Listings + Paid Ads: Add the paid ads previews to the boost product listings section #1650

Free Listings + Paid Ads: Add the paid ads previews to the boost product listings section #1650

Conversation

eason9487 commented Aug 24, 2022 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

📷 Text ellipsis

📷 Simulation of using product cover from merchant's DB

📷 Simulation of using shop logo from merchant's website

📹 All ad previews and blur animation

Detailed test instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope commented Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope commented Aug 29, 2022 • edited Loading

eason9487 commented Aug 30, 2022

puntope left a comment

Choose a reason for hiding this comment

eason9487 commented Aug 24, 2022 •

edited

Loading

puntope commented Aug 29, 2022 •

edited

Loading

puntope commented Aug 29, 2022 •

edited

Loading