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.

Sign up for GitHub

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

Jump to bottom

Add Campaigns card in Multichannel Marketing page #36735

Merged
merged 45 commits into from Mar 3, 2023
Merged

Add Campaigns card in Multichannel Marketing page #36735

merged 45 commits into from Mar 3, 2023

Conversation

ecgan
Copy link
Member

@ecgan ecgan commented Feb 2, 2023 •
edited

All Submissions:

Changes proposed in this Pull Request:

Closes #34905.

In this PR, we add a new Campaigns card into the Multichannel Marketing page. This builds on top of PR #36541, and uses the campaigns API introduced in PR #36222.

Campaigns card in loading state:

image

Campaigns card in error state:

image

Campaigns card in empty data state (no campaigns):

image

Campaigns card with campaign data and pagination:

image

Demo video showing pagination transition:

demo-pr-campaigns-card-pagination.mp4

Demo video showing the page transition when we click on the campaign link:

demo-pr-campaigns-card.mp4

How to test the changes in this Pull Request:

Pre-condition:

  • Enable WooCommerce Multichannel Marketing experience in WooCommerce Settings > Advanced > Features (/wp-admin/admin.php?page=wc-settings&tab=advanced&section=features).
  • Use Google Listings and Ads plugin from this PR Multichannel Marketing Integration google-listings-and-ads#1730.
  • Complete the setup onboarding in Google Listings and Ads and create a few ads campaigns. Alternatively, you can also follow the test instructions in PR Multichannel Marketing - API #36222 so that the campaigns API returns fake dummy campaigns data.

Test instructions:

  1. Go to Marketing page at /wp-admin/admin.php?page=wc-admin&path=%2Fmarketing.
  2. You should see the Campaigns card at the top of the page. The Campaigns card should display different states like the screenshots above.
    • You can modify useCampaigns hook to return the desired states for testing, e.g. loading: true, data: undefined, data: [] etc.
  3. Click on the next and previous pagination buttons. It should work as expected. See the demo video above for how it should work.
  4. Click on the campaign link. It should bring you to the campaign page quickly via client-side navigation.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@ecgan
Add empty state Campaigns card.
30d3168
@ecgan
Add loading state for Campaigns card.
af4f79a
@ecgan
Display campaign data in table in Campaigns card.
f5799c8
@ecgan
CSS for campaign rows in Campaigns card.
c444dbd
@ecgan
Add pagination to Campaigns card.
32cf8d1
@ecgan
Set cost column as numeric in Campaigns card.
cbc616a 
This makes the column right-aligned.
@ecgan
Add Campaign type.
c67e479
@ecgan
Use Campaign type in useCampaigns.
558ddf2
@ecgan
Merge branch 'feature/34903-multichannel-marketing-frontend/34906-cha…
ee6fa95 
…nnels-card' into feature/34903-multichannel-marketing-frontend/34905-campaigns-card

Conflicts:
	plugins/woocommerce-admin/client/marketing/overview-multichannel/MarketingOverviewMultichannel.tsx
@ecgan
Show Campaigns card only when there is at least one registered channel.
8e39098
@ecgan
Remove the unneeded "Create new campaign" button for now.
1d7b663
@ecgan
Merge branch 'feature/34906-marketing-channels-card' into feature/349…
7610214 
…05-marketing-campaigns-card

Conflicts:
	plugins/woocommerce-admin/client/marketing/overview-multichannel/MarketingOverviewMultichannel.tsx
@ecgan
Fix ids typescript error in Campaigns.
d69f7f0
@ecgan
Remove unused import.
3384ad5
@ecgan
Merge branch 'feature/34906-marketing-channels-card' into feature/349…
630bd0a 
…05-marketing-campaigns-card
@ecgan
Fetch and display campaigns with wp.data.
cd7b5f2
@ecgan
Change to color variable in Campaigns CSS.
c342cbb
@ecgan
Check description before rendering FlexItem.
2915442 
This is to eliminate the unneeded flex gap when there is no description.
@ecgan
Use Link component in Campaigns.
274ea3d 
This allows fast client-side navigation.
@ecgan
Use perPage constant instead of literal 5.
eda6f19
@ecgan
Show CardFooter and Pagination only when it is needed.
183fc48 
This means when total is more than perPage.
@ecgan
Set per_page to 100 for getCampaigns.
743c0e5 
100 is the maximum limit allowed by the API.

We do this because the API does not return total number of rows, so we use 100 to get "all" rows, to support pagination in the UI.
@ecgan
Display error state in Campaigns card.
0abe368
@ecgan
Code refactor for Campaigns card.
c17f336
@ecgan
Code refactor for Campaigns card CSS.
7b068bf
@ecgan ecgan added the focus: marketing Marketing page in WooCommerce Admin, i.e. `/wp-admin/admin.php?page=wc-admin&path=%2Fmarketing`. label Feb 2, 2023
@ecgan ecgan requested review from a team February 2, 2023 10:34
@ecgan ecgan self-assigned this Feb 2, 2023
puntope
puntope reviewed Feb 28, 2023
View reviewed changes
plugins/woocommerce-admin/client/marketing/data-multichannel/selectors.ts
@@ -10,3 +10,7 @@ export const getRegisteredChannels = ( state: State ) => {
export const getRecommendedChannels = ( state: State ) => {
return state.recommendedChannels;
};

export const getCampaigns = ( state: State ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Missed JSDOCS

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 4b87f0a (#36735).

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed @param state declaration

puntope
puntope reviewed Feb 28, 2023
View reviewed changes
plugins/woocommerce-admin/client/marketing/data-multichannel/types.ts Outdated
};

export type CampaignsState = {
error?: ApiFetchError;
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Whats the difference between the error in CampaignsState vs an error in CampaignsPage ??

Copy link
Member Author

@ecgan ecgan Mar 1, 2023 •
edited

Choose a reason for hiding this comment

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

That is actually unused now, I have removed it in c0364ad (#36735).

puntope
puntope reviewed Feb 28, 2023
View reviewed changes
Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

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

Howdy @ecgan

Behaviour testing good. Left some comments over code.

@ecgan ecgan requested a review from puntope March 1, 2023 19:54
@ecgan
Copy link
Member Author

ecgan commented Mar 1, 2023

@puntope , thanks for the review comments, I have made changes and addressed them, this is ready for another round of review. 🙂 🙏

PS: ❤️ Extra special thanks for #36735 (review), I got what I wanted to do based on your code example!

@ecgan ecgan mentioned this pull request Mar 2, 2023
Merged
9 tasks
puntope
puntope reviewed Mar 3, 2023
View reviewed changes
plugins/woocommerce-admin/client/marketing/data-multichannel/actions.ts
/**
* Create a "RECEIVE_CAMPAIGNS" action object.
*/
export const receiveCampaigns = ( response: CampaignsResponse ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

💅 Missed @param response docs

puntope
puntope reviewed Mar 3, 2023
View reviewed changes
plugins/woocommerce-admin/client/marketing/data-multichannel/resolvers.ts
* Get total number of records from the HTTP response header "x-wp-total".
*
* If the header is not present, then the function will return `undefined`.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed param response docs

puntope
puntope approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

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

Looking good for me @ecgan

Thanks for further changes

@ecgan ecgan merged commit 00a12db into trunk Mar 3, 2023
@ecgan ecgan deleted the feature/34905-marketing-campaigns-card branch March 3, 2023 10:43
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nima-karimi nima-karimi nima-karimi left review comments

@puntope puntope puntope approved these changes

Assignees

@ecgan ecgan

Labels
focus: marketing Marketing page in WooCommerce Admin, i.e. `/wp-admin/admin.php?page=wc-admin&path=%2Fmarketing`. plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Milestone
7.6.0
Development

Successfully merging this pull request may close these issues.

Add the "Campaigns" card into Marketing page
3 participants
@ecgan @puntope @nima-karimi