Skip to content

Add product card styling. #30410

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
madeincosmos merged 6 commits into from
Aug 11, 2021
Merged

Add product card styling. #30410

madeincosmos merged 6 commits into from
Aug 11, 2021

Conversation

@madeincosmos
Copy link
Contributor

@madeincosmos madeincosmos commented Aug 3, 2021 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

  • Update product cards on the WooCommerce > Extensions page in WP Admin to match current designs.

How to test the changes in this Pull Request:

  1. Check out this branch on local environment.
  2. Navigate to one of marketplace category pages, i.e. wp-admin/admin.php?page=wc-addons&section=payment-gateways (nav menu isn't implemented at the moment
  3. Check product card and make sure it looks good on all screen sizes

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? - e2e tests are currently failing because the WooCommerce.com Subscriptions screen is now in a different place, these changes are addressed in Split the Extensions page in WP Admin and add respective menu items #30380.
  • Have you successfully run tests with your changes locally?

Screenshots

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.

@madeincosmos madeincosmos self-assigned this Aug 3, 2021
@madeincosmos madeincosmos added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Aug 3, 2021
andfinally
andfinally reviewed Aug 4, 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 great, thanks! There are a few things I still need to check. I think we probably need to retain the max-width 1200px on the list of products and align it left, like in the current page. So we may want to adjust the width of the cards a little bit to allow for that greater width.

assets/css/admin.scss Outdated
inset 0 -1px 0 rgba(0, 0, 0, 0.1);
padding: 0;
vertical-align: top;
width: 45%;
Copy link
Contributor

@andfinally andfinally Aug 4, 2021

Choose a reason for hiding this comment

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

We should probably make this

max-width: 45%;

to stop any trailing products from taking full width like this:

image

Copy link
Contributor Author

@madeincosmos madeincosmos Aug 5, 2021

Choose a reason for hiding this comment

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

Added max width on product card and an extra wrapper to make the product list look good on all screen sizes:

@tgglv
Copy link
Contributor

tgglv commented Aug 9, 2021

Hey @madeincosmos, just started to see this PR and noticed that E2E tests failed:

a.nav-tab (text: "WooCommerce.com Subscriptions") not found

It looks like the expected element is missed. It might happen coincidentally. Can you take a look?

@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 Aug 9, 2021
@madeincosmos
Copy link
Contributor Author

madeincosmos commented Aug 9, 2021

Hey @madeincosmos, just started to see this PR and noticed that E2E tests failed:

Hi @tgglv, the e2e tests are failing because we changed the marketplace page structure. I've updated the tests in a different branch that is currently merged into update/marketplace-1, I can try to rebase this one if that's needed.

@andfinally
Merge branch 'update/marketplace-1' into update/styling-product-cards
1ec1f25
@andfinally
CSS and layout tweaks.
a44d9c9 
- Changed `wc_addons_wrap` to `wc-addons-wrap` to adhere to stylelint class rule.
- Removed `max-width` from `wc-addons-wrap` – we need the purple header to stretch to full width. Added new `wc-subscriptions-wrap` class to give the `max-width` to the My Subscriptions page.
- Changed width of product cards to use `calc`, so that any trailing odd-numbered card has the same width as the others.
- Fixed some stylelint issues. Moved style blocks to address `no-descending-specificity` warnings. Remove quotes from URLs and font names that don't need them.
- Gave `#screen-meta-links` absolute position on the marketplace page, so it overlays the full-width purple header.
@andfinally
Addressing PHPCS errors.
0ceaf93 
- Deleted precision indentation on lines 72 and 80.
- Deleted unnecessary `echo` from before `esc_html_e` cals.
andfinally
andfinally approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@andfinally andfinally 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.

Looks lovely! My apologies Maria, I pushed some additions to this PR before I saw your comment, hope this is consistent with your work. I made some tweaks to the page layout.

  • Made the wrapper full width, so the purple header stretches across the screen.
  • Aligned the products to the left.
  • Used calc for product card width, so any trailing odd-numbered card keeps the same width as the others.
  • Addressed some stylelint warnings in admin.scss.

The PHP7.4 WP Nightly test is repeatedly failing, something to do with composer dependencies – I don't believe it's related to the changes in this PR.

@madeincosmos
Copy link
Contributor Author

madeincosmos commented Aug 10, 2021

Thanks @andfinally, I like your approach better than the previous calculations based on margins. I can see in the designs that there's 84px space between WordPress menu and product list, so I've added this on larger screens. Other than this, I think we're good to.

@madeincosmos madeincosmos merged commit 662af41 into update/marketplace-1 Aug 11, 2021
@madeincosmos madeincosmos deleted the update/styling-product-cards branch August 11, 2021 10:30
@github-actions github-actions bot added this to the 5.7.0 milestone Aug 11, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2021

Hi @madeincosmos, 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

@andfinally andfinally mentioned this pull request Aug 13, 2021
Merged
6 tasks
@roykho roykho added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Aug 17, 2021
@rodelgc rodelgc added testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Aug 24, 2021
@tgglv tgglv mentioned this pull request Sep 30, 2021
Merged
7 tasks
@corsonr corsonr mentioned this pull request Oct 1, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kloon kloon Awaiting requested review from kloon

@corsonr corsonr Awaiting requested review from corsonr

1 more reviewer

@andfinally andfinally andfinally approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@madeincosmos madeincosmos

Labels

needs: triage feedback Issues for which we requested feedback from the author and received it. release: add changelog Mark all PRs that have not had their changelog entries added. [auto]

Projects

None yet

Milestone

5.7.0

Development

Successfully merging this pull request may close these issues.

5 participants

@madeincosmos @tgglv @andfinally @roykho @rodelgc