Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Fixes to Twenty Twenty product grid styles #2573

Merged
merged 5 commits into from Jun 2, 2020
Merged

Fixes to Twenty Twenty product grid styles #2573

merged 5 commits into from Jun 2, 2020

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented May 27, 2020

Fixes #2537.
Fixes #2421.

Related pull in Core: woocommerce/woocommerce#26608.

Screenshots

Hand-Picked Products

Before:
imatge

After:
imatge

All Products

Before:
imatge

After:
imatge
(notice there are some alignment/marging issues, but those are being handled/tracked in woocommerce/woocommerce#26516 and #2565)

How to test the changes in this Pull Request:

  1. Switch to the Twenty Twenty theme.
  2. Add an All Products block, edit it and add an On Sale badge below the title.
  3. Verify it's correctly positioned.
  4. Add a Hand-picked Products block and select a product on sale.
  5. Verify the discounted price is not underlined.

Changelog

Fixes to the product grid blocks in Twenty Twenty: discounted prices are no longer underlined and the On Sale badge is correctly positioned in the All Products block.

@Aljullu Aljullu added this to the 2.7.0 milestone May 27, 2020
@Aljullu Aljullu requested a review from a team as a code owner May 27, 2020 11:34
@Aljullu Aljullu self-assigned this May 27, 2020
@Aljullu Aljullu requested review from haszari and removed request for a team May 27, 2020 11:34
@github-actions
Copy link
Contributor

github-actions bot commented May 27, 2020 •
edited

Size Change: +275 B (0%)

Total Size: 1.78 MB

Filename Size Change
build/all-products-frontend.js 71.6 kB +9 B (0%)
build/all-products.js 14.7 kB +10 B (0%)
build/single-product.js 13.9 kB +13 B (0%)
build/style-legacy-rtl.css 5.7 kB +71 B (1%)
build/style-legacy.css 5.7 kB +70 B (1%)
build/style-rtl.css 16.6 kB +50 B (0%)
build/style.css 16.6 kB +52 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/active-filters-frontend.js 7.26 kB 0 B
build/active-filters.js 7.89 kB 0 B
build/all-reviews-legacy.js 9.19 kB 0 B
build/all-reviews.js 9.46 kB 0 B
build/attribute-filter-frontend.js 16.7 kB 0 B
build/attribute-filter.js 11.5 kB 0 B
build/block-error-boundary-legacy.js 774 B 0 B
build/block-error-boundary.js 775 B 0 B
build/blocks-legacy.js 2.92 kB 0 B
build/blocks.js 2.92 kB 0 B
build/cart-frontend.js 113 kB 0 B
build/cart.js 32.3 kB 0 B
build/checkout-frontend.js 129 kB 0 B
build/checkout.js 37.9 kB 0 B
build/custom-select-control-style-legacy.js 782 B 0 B
build/custom-select-control-style.js 782 B 0 B
build/editor-legacy-rtl.css 12.5 kB 0 B
build/editor-legacy.css 12.5 kB 0 B
build/editor-rtl.css 13.5 kB 0 B
build/editor.css 13.5 kB 0 B
build/featured-category-legacy.js 7.26 kB 0 B
build/featured-category.js 7.54 kB 0 B
build/featured-product-legacy.js 9.5 kB 0 B
build/featured-product.js 9.78 kB 0 B
build/handpicked-products-legacy.js 6.9 kB 0 B
build/handpicked-products.js 7.18 kB 0 B
build/panel-style-legacy.js 773 B 0 B
build/panel-style.js 774 B 0 B
build/price-filter-frontend.js 14.1 kB 0 B
build/price-filter.js 9.98 kB 0 B
build/product-best-sellers-legacy.js 6.98 kB 0 B
build/product-best-sellers.js 7.25 kB 0 B
build/product-categories-legacy.js 3.22 kB 0 B
build/product-categories.js 3.21 kB 0 B
build/product-category-legacy.js 7.89 kB 0 B
build/product-category.js 8.15 kB 0 B
build/product-list-style-legacy.js 775 B 0 B
build/product-new-legacy.js 7.13 kB 0 B
build/product-new.js 7.41 kB 0 B
build/product-on-sale-legacy.js 7.5 kB 0 B
build/product-on-sale.js 7.8 kB 0 B
build/product-search-legacy.js 3.12 kB 0 B
build/product-search.js 3.36 kB 0 B
build/product-tag-legacy.js 6.07 kB 0 B
build/product-tag.js 6.33 kB 0 B
build/product-top-rated-legacy.js 7.11 kB 0 B
build/product-top-rated.js 7.38 kB 0 B
build/products-by-attribute-legacy.js 7.87 kB 0 B
build/products-by-attribute.js 8.13 kB 0 B
build/reviews-by-category-legacy.js 11.2 kB 0 B
build/reviews-by-category.js 11.5 kB 0 B
build/reviews-by-product-legacy.js 12.7 kB 0 B
build/reviews-by-product.js 13 kB 0 B
build/reviews-frontend-legacy.js 8.03 kB 0 B
build/reviews-frontend.js 8.95 kB 0 B
build/single-product-frontend.js 58.9 kB 0 B
build/snackbar-notice-style-legacy.js 778 B 0 B
build/snackbar-notice-style.js 778 B 0 B
build/spinner-style-legacy.js 775 B 0 B
build/spinner-style.js 771 B 0 B
build/vendors-legacy.js 376 kB 0 B
build/vendors-style-legacy-rtl.css 1.65 kB 0 B
build/vendors-style-legacy.css 1.65 kB 0 B
build/vendors-style-legacy.js 105 B 0 B
build/vendors-style-rtl.css 1.65 kB 0 B
build/vendors-style.css 1.65 kB 0 B
build/vendors-style.js 105 B 0 B
build/vendors.js 473 kB 0 B
build/wc-blocks-data.js 7.08 kB 0 B
build/wc-blocks-middleware.js 932 B 0 B
build/wc-blocks-registry.js 1.78 kB 0 B
build/wc-payment-method-cheque.js 794 B 0 B
build/wc-payment-method-paypal.js 830 B 0 B
build/wc-payment-method-stripe.js 11.9 kB 0 B
build/wc-settings.js 2.14 kB 0 B
build/wc-shared-context.js 1.18 kB 0 B

compressed-size-action

@haszari
Copy link
Member

haszari commented May 27, 2020 •
edited

I'm a little confused - those screenshots are a real spot-the-difference 🤣

Summarising the changes:

  1. Remove spurious underline of discounted prices in product grid blocks, for consistency with regular prices.
  2. Show sale badge near prices (not overlaid on image) in product grid blocks.

(1) makes total sense to me.

(2) I don't understand – I'd assume we'd consistently show the sale badge overlaying the image on top-right corner. Is there a reason why we want it in a different position?

I get it now :)

  • there's an On-Sale Badge block that merchant can use anywhere in All Products
  • this is separate to the Show On-Sale Badge option for the product image block
  • the bug is that this On Sale Badge block is incorrectly positioned

Bug:

Screen Shot 2020-05-28 at 9 21 29 AM

haszari
haszari reviewed May 27, 2020
View reviewed changes
Copy link
Member

@haszari haszari left a comment

Choose a reason for hiding this comment

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

The underlining fix looks good :)

However I think there's still an issue with the default sale badge in product image. This was just a quick test in Chrome, I'm happy to retest across browsers when this issue is fixed.

FYI it took a little bit of research/experimentation to understand the problem, I've added a comment with my understanding.

assets/js/base/components/product-list/style.scss
}

// These styles are not applied to the All Products atomic block, so it can be positioned normally.
.wc-block-grid__products .wc-block-grid__product-onsale:not(.wc-block-component__sale-badge) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we need to specifically target the image overlay sale badge to get it top-right. It's not working for me in Chrome:

Screen Shot 2020-05-28 at 9 24 42 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I missed that. Should be fixed in 940ab69.

Copy link
Member

Choose a reason for hiding this comment

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

Dang - I'm still seeing the issue?!

Screen Shot 2020-05-29 at 3 14 45 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, my bad, I forgot they could be aligned to the left and to the center. The CSS selector I wrote was only targeting them when they were right-aligned. 🤦

Should be fixed with 20b9ea4.

@Aljullu
Copy link
Contributor Author

Aljullu commented May 28, 2020

Sorry for not being clear enough, some times when working on an issue I think other people will be in my mind. 😅

So yes, this PR is about: removing the discounted price underline (#2421) and allowing the All Products sale badge inner block to be positioned normally (#2537).

I updated this PR fixing the reported issue. Willing to take another look @haszari?

@Aljullu Aljullu requested a review from haszari May 28, 2020 12:10
@haszari haszari force-pushed the fix/twentytwenty-grid-styles branch from 20b9ea4 to 5bd32ce Compare June 2, 2020 03:21
@haszari
Copy link
Member

haszari commented Jun 2, 2020

FYI @Aljullu I rebased this :)

haszari
haszari approved these changes Jun 2, 2020
View reviewed changes
Copy link
Member

@haszari haszari 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 - tested a few variations in Firefox, Chrome and Safari and saw no issues. Thanks @Aljullu :)

@Aljullu Aljullu merged commit 9cb2c96 into master Jun 2, 2020
@Aljullu Aljullu deleted the fix/twentytwenty-grid-styles branch June 2, 2020 07:18
@Aljullu Aljullu added the type: bug The issue/PR concerns a confirmed bug. label Jun 8, 2020
@Aljullu Aljullu mentioned this pull request Jun 8, 2020
Merged
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@haszari haszari haszari approved these changes

Assignees

@Aljullu Aljullu

Labels
type: bug The issue/PR concerns a confirmed bug.
Projects
None yet
Milestone
2.7.0
3 participants
@Aljullu @haszari @woocommercebot