PMax Assets: Support the portrait marketing images #1873

eason9487 · 2023-02-08T04:35:07Z

Changes proposed in this Pull Request:

This PR implements the "Assets form" part of 📌 Assets form and related components in #1787.

Support the portrait marketing image and adjust the maximum number of each marketing image type to make the sum of the maximum limits 20.

The limits are adjusted to:

MARKETING_IMAGE: min 1, max 8
SQUARE_MARKETING_IMAGE: min 1, max 8
PORTRAIT_MARKETING_IMAGE: min 0, max 4
- Image size: (4:5) 960 x 1200 recommended; 480 x 600 min

Screenshots:

📷 Added portrait marketing image asset and adjusted maximum limits

📷 Validation of the portrait marketing image asset

Detailed test instructions:

Go to the campaign creation or editing page.
Check if the limits of each marketing image type match the proposed solution.
Check if the UI of the portrait marketing image asset work well.
Click the "Save changes" button to see if the validation of all marketing image types works correctly.

Additional details:

Reason for adjustment: pcTzPl-12o-p2#comment-2228
Adopted solution for the present: pcTzPl-12o-p2#comment-2247

Changelog entry

jorgemd24

LGTM 👍

I tested locally and can see that the validation is working and the image gets cropped to the correct size.

Other things/opinions beyond this PR:

I think the delete button looks too big compared with the preview images, what do you think? Or comparing with the Figma file, it seems that the preview images should be bigger.

When I am in the editing asset section and I refresh the page, I get back to step one, shouldn't we get back to the editing asset section?
Do you think we need the button "Scan for assets"? After I choose the Final URL, the only option possible is to scan for the assets, so I would expect to do this automatically.

jorgemd24 · 2023-02-08T11:08:23Z

js/src/data/types.js

 * @property {string[]} marketing_images The URLs of the landscape images.
 * @property {string[]} square_marketing_images The URLs of the square images.


Not introduced in this PR, and probably it was updated when I started to use the field types from the Google Ads library.

marketing_images and square_marketing_images should be in singular: marketing_image and square_marketing_image

Thanks for reminding me. Updated in 3736430.

jorgemd24 · 2023-02-08T11:08:45Z

js/src/data/types.js

 * @property {AssetEntity[]} [marketing_images] The URLs of the landscape images.
 * @property {AssetEntity[]} [square_marketing_images] The URLs of the square images.


See previous message.

Thanks for catching this. Fixed in 3736430.

…es of AssetsDictionary to align with the correct names. Address - #1873 (comment) - #1873 (comment)

eason9487 · 2023-02-09T03:21:47Z

@jorgemd24, thanks for the review.

I think the delete button looks too big compared with the preview images, what do you think? Or comparing with the Figma file, it seems that the preview images should be bigger.

Could you provide the WP and WC versions you used for testing? From the screenshot, it seems that there were many styles that were overwritten or not applied to buttons and images. This might be a compatible issue or some styles were not loaded correctly for some reason.

The styles on my local env look like this:

When I am in the editing asset section and I refresh the page, I get back to step one, shouldn't we get back to the editing asset section?

I guess you were on the campaign creation page. Only the campaign editing page will stay on the same step after refreshing the page. The creation page doesn't resume the current step because it doesn't keep the entered form values on the first step so it requires the user to re-fill from the first step.

Do you think we need the button "Scan for assets"? After I choose the Final URL, the only option possible is to scan for the assets, so I would expect to do this automatically.

If I understood it right, it tended to align with the same Card UI pattern of other similar features in GLA, so I think it should be fine. I will forward it to the designer to see if it needs to be adjusted.

jorgemd24 · 2023-02-09T10:33:16Z

Could you provide the WP and WC versions you used for testing? From the screenshot, it seems that there were many styles that were overwritten or not applied to buttons and images. This might be a compatible issue or some styles were not loaded correctly for some reason.

WP version: 6.1.1
WC version: 7.3.0

Let me know if you can reproduce it with those versions, otherwise, I will look deeper into the issue.

I guess you were on the campaign creation page. Only the campaign editing page will stay on the same step after refreshing the page. The creation page doesn't resume the current step because it doesn't keep the entered form values on the first step so it requires the user to re-fill from the first step.

It happens when I click on the editing campaign button, see this video:

Screen.Capture.on.2023-02-09.at.11-30-04.mp4

If I understood it right, it tended to align with the same Card UI pattern of other similar features in GLA, so I think it should be fine.

👍

jorgemd24 · 2023-02-09T10:43:11Z

The conflict with the styling is with AutomateWoo 5.6.6 when I disable AW loads correctly.

eason9487 · 2023-02-09T11:54:35Z

@jorgemd24, thanks for the seconde round and the more infomation!

It happens when I click on the editing campaign button, [...]

You are right, it's a bug. I previously tested it with another "Edit" button on the campaign table, which is for demo purpose. 🤦‍♂️ I opened PR #1877 for this.

The conflict with the styling is with AutomateWoo 5.6.6 when I disable AW loads correctly.

Nice finding! I guess it relates to #1391.

… each marketing image type to make the sum of the maximum limits 20.

…es of AssetsDictionary to align with the correct names. Address - #1873 (comment) - #1873 (comment)

eason9487 · 2023-02-15T03:51:58Z

Hi @jorgemd24, I got the update on this question.

Do you think we need the button "Scan for assets"? After I choose the Final URL, the only option possible is to scan for the assets, so I would expect to do this automatically.

If I understood it right, it tended to align with the same Card UI pattern of other similar features in GLA, so I think it should be fine. I will forward it to the designer to see if it needs to be adjusted.

After checking with the designer, the decision is to stay with the current UI, and here are the reasons copied from the conversation:

Actually the reason we did the button was to be consistent about the pattern. We use a similar pattern for connecting accounts. We want to share feedback about what exactly is happening after users select a page.
Since we only have 1 flow for the MVP version (change URL + import assets), I’m thinking to keep things as is for now, or simplify the wording and use “Select”. B/c in the future, changing the URL won't always mean “scan for assets”.

jorgemd24 · 2023-02-15T09:24:39Z

Thanks so much for the explanation @eason9487 !

eason9487 requested a review from a team February 8, 2023 04:35

eason9487 self-assigned this Feb 8, 2023

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Feb 8, 2023

eason9487 mentioned this pull request Feb 8, 2023

PMax Assets - Stage 1 #1787

Closed

34 tasks

jorgemd24 approved these changes Feb 8, 2023

View reviewed changes

eason9487 added a commit that referenced this pull request Feb 9, 2023

Update the property names of SuggestedAssets and fix the property nam…

3736430

…es of AssetsDictionary to align with the correct names. Address - #1873 (comment) - #1873 (comment)

eason9487 mentioned this pull request Feb 9, 2023

PMax Assets: Preprocessing for API integrations #1872

Merged

eason9487 mentioned this pull request Feb 9, 2023

PMax Assets: Stay on the same step after refreshing the campaign editing page #1877

Merged

eason9487 added 3 commits February 13, 2023 11:46

Support the portrait marketing image and adjust the maximum number of…

fe61cf0

… each marketing image type to make the sum of the maximum limits 20.

Update the test for the validateAssetGroup function

326402c

Update the property names of SuggestedAssets and fix the property nam…

cbcabcc

…es of AssetsDictionary to align with the correct names. Address - #1873 (comment) - #1873 (comment)

eason9487 force-pushed the update/1787-support-portrait-marketing-image branch from 3736430 to cbcabcc Compare February 13, 2023 03:55

Base automatically changed from add/1787-preprocessing-integrate-api to feature/pmax-assets February 14, 2023 04:28

eason9487 merged commit 583af8b into feature/pmax-assets Feb 14, 2023

eason9487 deleted the update/1787-support-portrait-marketing-image branch February 14, 2023 04:30

eason9487 mentioned this pull request Feb 28, 2023

PMax Assets: Prevent the button styles in ImagesSelector from overriding by AutomateWoo #1909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PMax Assets: Support the portrait marketing images #1873

PMax Assets: Support the portrait marketing images #1873

eason9487 commented Feb 8, 2023

jorgemd24 left a comment •

edited

Loading

jorgemd24 Feb 8, 2023 •

edited

Loading

eason9487 Feb 9, 2023

jorgemd24 Feb 8, 2023

eason9487 Feb 9, 2023

eason9487 commented Feb 9, 2023

jorgemd24 commented Feb 9, 2023

jorgemd24 commented Feb 9, 2023

eason9487 commented Feb 9, 2023

eason9487 commented Feb 15, 2023

jorgemd24 commented Feb 15, 2023

		* @property {string[]} marketing_images The URLs of the landscape images.
		* @property {string[]} square_marketing_images The URLs of the square images.

		* @property {AssetEntity[]} [marketing_images] The URLs of the landscape images.
		* @property {AssetEntity[]} [square_marketing_images] The URLs of the square images.

PMax Assets: Support the portrait marketing images #1873

PMax Assets: Support the portrait marketing images #1873

Conversation

eason9487 commented Feb 8, 2023

Changes proposed in this Pull Request:

Screenshots:

📷 Added portrait marketing image asset and adjusted maximum limits

📷 Validation of the portrait marketing image asset

Detailed test instructions:

Additional details:

Changelog entry

jorgemd24 left a comment • edited Loading

Choose a reason for hiding this comment

jorgemd24 Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

eason9487 Feb 9, 2023

Choose a reason for hiding this comment

jorgemd24 Feb 8, 2023

Choose a reason for hiding this comment

eason9487 Feb 9, 2023

Choose a reason for hiding this comment

eason9487 commented Feb 9, 2023

jorgemd24 commented Feb 9, 2023

jorgemd24 commented Feb 9, 2023

eason9487 commented Feb 9, 2023

eason9487 commented Feb 15, 2023

jorgemd24 commented Feb 15, 2023

jorgemd24 left a comment •

edited

Loading

jorgemd24 Feb 8, 2023 •

edited

Loading