Skip to content

In-App Marketplace Category Banners #30938

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
Merged
claudiosanches merged 7 commits into from
Oct 13, 2021
Merged

In-App Marketplace Category Banners #30938

claudiosanches merged 7 commits into from
Oct 13, 2021

Conversation

@kloon
Copy link
Contributor

@kloon kloon commented Oct 13, 2021 •
edited by claudiosanches
Loading

All Submissions:

Changes proposed in this Pull Request:

Remove the hardcoded WC Pay and WC Services banners on the Marketplace categories pages and transform the system to use the WCCOM API and the WC Admin Rule Evaluator class for determining if a banner should be displayed on these pages.

Closes 11309-gh-Automattic/woocommerce.com

How to test the changes in this Pull Request:

  1. Check out this branch
  2. Ensure your store country is set to US WooCommerce -> Settings
  3. In WP-Admin visit WooCommerce -> Marketplace and verify products are still shown
  4. Now navigate to the Payments category in WooCommerce -> Marketplace and verify the WC Payments banner is displayed above the products, if you have WC Payments installed it should not show so test with the plugin activated and deactivated.
  5. Now navigate to the Shipping category in WooCommerce -> Marketplace and verify the WC Services banner is displayed above the products, if you have WC Services installed already it should not show so test with the plugin activated and deactivated.

Other information:

These changes form part of the In-App Marketplace revamp.

  • 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 successfully run tests with your changes locally?

Changelog entry

Tweak - Remove hardcode category banners in Settings > Marketplace and use the WooCommerce.com API instead.

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.

@kloon kloon added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Oct 13, 2021
@kloon kloon added this to the In-App Marketplace milestone Oct 13, 2021
@kloon kloon requested review from a team, andfinally and tgglv October 13, 2021 11:40
@kloon kloon self-assigned this Oct 13, 2021
@kloon kloon requested review from claudiosanches and removed request for a team October 13, 2021 11:40
@kloon kloon changed the title Add/wccom 11309 In-App Marketplace Category Banners Oct 13, 2021
@madeincosmos
Copy link
Contributor

madeincosmos commented Oct 13, 2021

Thanks for the PR! The banner on the Payments page is looking good on all screen sizes, the Shipping one will require a fix in category names API - currently I can see it when I manually enter this URL:

https://wp.test:4443/wp-admin/admin.php?page=wc-addons&section=shipping-methods

I'm also having problems with the shipping banner image not loading correctly from https://wp.test:4443/wp-content/plugins/woocommerce/assets/images/wcs-extensions-banner-3x.png

@github-actions github-actions bot added needs: triage feedback Issues for which we requested feedback from the author and received it. and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Oct 13, 2021
@kloon
Copy link
Contributor Author

kloon commented Oct 13, 2021

Thanks Maria, PR with API fixes ready hereOnce that is merged the issues should be resolved in this PR.

claudiosanches
claudiosanches reviewed Oct 13, 2021
View reviewed changes
claudiosanches
claudiosanches reviewed Oct 13, 2021
View reviewed changes
claudiosanches
claudiosanches previously approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@claudiosanches claudiosanches left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works great!

I made some small coding standards fixes.

Just note that get_extension_data() is breaking compatibility now, but since it's used only as internal class I don't expect any issue from it.

@kloon
Copy link
Contributor Author

kloon commented Oct 13, 2021

Thanks, Claudio!

andfinally
andfinally previously approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@andfinally andfinally 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, thanks!

@andfinally
Copy link
Contributor

andfinally commented Oct 13, 2021

Timur said how he got the tests working on his PR:

Solved the problem with tests by changing hash in the composer.lock. It looks like coincidentally phpunit files in cache break running composer install.

@kloon kloon dismissed stale reviews from andfinally and claudiosanches via d5c3d6d October 13, 2021 16:00
@kloon kloon removed the request for review from tgglv October 13, 2021 16:39
@andfinally andfinally mentioned this pull request Oct 13, 2021
Closed
claudiosanches
claudiosanches approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@claudiosanches claudiosanches left a comment

Choose a reason for hiding this comment

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

Looks and works great now!

@claudiosanches claudiosanches merged commit a1989d1 into trunk Oct 13, 2021
@claudiosanches claudiosanches deleted the add/wccom-11309 branch October 13, 2021 16:59
@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2021 •
edited by claudiosanches
Loading

Hi @claudiosanches, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the status: needs changelog label
  • Add the status: needs testing instructions label

@claudiosanches claudiosanches added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] and removed needs: triage feedback Issues for which we requested feedback from the author and received it. labels Oct 13, 2021
@Konamiman Konamiman added changelog added and removed release: add changelog Mark all PRs that have not had their changelog entries added. [auto] labels Oct 15, 2021
@tammullen tammullen added release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@claudiosanches claudiosanches claudiosanches approved these changes

@andfinally andfinally andfinally left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kloon kloon

Labels

None yet

Projects

None yet

Milestone

5.9.0

Development

Successfully merging this pull request may close these issues.

7 participants

@kloon @madeincosmos @andfinally @claudiosanches @Konamiman @tammullen