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.

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

Product card spacing and price polish #407

Merged
merged 3 commits into from
Aug 18, 2021
Merged

Conversation

kmeleta
Copy link
Contributor

@kmeleta kmeleta commented Aug 16, 2021

Why are these changes introduced?

Fixes #231

Product card spacing:

  • The space between the image and the first element should be 2rem (now 1.3 mobile/1.7 desktop)
  • Space between vendor and title should reduce from 1.1rem to 0.7rem
  • The space between title and price should be 1.1rem (now 0.7rem)

Update on sale regular price (strikethrough) font size:

  • Product card 1.3rem
  • Collage product card 1.3rem
  • Product page 1.3rem (mobile) 1.6rem (desktop)
  • Customer order page 1.3rem

Product page:

What approach did you take?

I chose to update the regular price font size to 1.3rem directly in the price component and add a product page specific override that would bump this to 1.6rem on desktop.

For the customer order template, I added some classes to help target elements in the price columns specifically. This also fixes a bug where the on sale/regular price was unintentionally receiving the unit price styles. This section should now look like this.

Demo links

Checklist

@kmeleta kmeleta marked this pull request as ready for review August 16, 2021 17:23
@Oliviammarcello
Copy link
Contributor

Oliviammarcello commented Aug 16, 2021

Few updates:

  • Remove 0.4rem of padding-top above price on both desktop and mobile. Screenshot
  • When the unit price is enabled there should be more space between the price and the sale badge as it is resizing. Screenshot
  • Space between product image and title should be updated to 1.3rem on mobile. Screenshot.On desktop, it should change from 2rem to 1.7rem. Screenshot. Sorry for the second update here.

Thanks!

@@ -262,6 +262,7 @@ a.product__text {

.product .price {
align-items: flex-start;
gap: 1rem;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gap is to fix this issue which only happens at specific viewport widths for on sale items with unit pricing. I chose 1rem so it can take the place of the existing horizontal 1rem margin between the sale price and the sale badge (when displayed next to each on larger breakpoints).

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this clean approach! But some versions of the latest iOS Safari 14 release (< 14.4) don't support this.

Screen Shot 2021-08-17 at 2 12 08 PM

Screen_Shot_2021-08-17_at_2_07_32_PM

@ludoboludo ludoboludo self-requested a review August 17, 2021 13:27
ludoboludo
ludoboludo previously approved these changes Aug 17, 2021
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@@ -129,7 +129,7 @@
<dt>
<span class="visually-hidden">{{ 'products.product.price.regular_price' | t }}</span>
</dt>
<dd>
<dd class="regular-price">
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember wanting to stir away from using classes in the customer templates. Those classes should be generic enough so it's ok to have them I take it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think so. We do use a few other simple classes in this template as well as the account table for convenience and clarity. At a certain point a selector becomes too flaky and indiscernible to justify. This PR actually resolves a styling bug that presumably happened due to unintended selector scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet! And makes sense some selectors you removed were not pretty 😅

@KaichenWang KaichenWang self-assigned this Aug 17, 2021
KaichenWang
KaichenWang previously approved these changes Aug 17, 2021
Copy link
Contributor

@KaichenWang KaichenWang left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few comments

line-height: 1.2;
margin-top: 0.2rem;
text-transform: uppercase;
color: var(--color-foreground-70);
Copy link
Contributor

Choose a reason for hiding this comment

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

--color-foreground-XX CSS variables no longer exist in the theme. More context here.

Suggested change
color: var(--color-foreground-70);
color: rgba(var(--color-foreground), 0.7);

@@ -262,6 +262,7 @@ a.product__text {

.product .price {
align-items: flex-start;
gap: 1rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this clean approach! But some versions of the latest iOS Safari 14 release (< 14.4) don't support this.

Screen Shot 2021-08-17 at 2 12 08 PM

Screen_Shot_2021-08-17_at_2_07_32_PM

@kmeleta kmeleta force-pushed the product-card-price-polish branch from ed6a3c3 to 42da681 Compare August 17, 2021 19:44
@@ -34,7 +34,8 @@
{%- if price_class %} {{ price_class }}{% endif -%}
{%- if available == false %} price--sold-out {% endif -%}
{%- if compare_at_price > price %} price--on-sale {% endif -%}
{%- if product.price_varies == false and product.compare_at_price_varies %} price--no-compare{% endif -%}">
{%- if product.price_varies == false and product.compare_at_price_varies %} price--no-compare {% endif -%}
{%- if product.selected_or_first_available_variant.unit_price_measurement != nil %} price--unit-price{% endif -%}">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

But some versions of the latest iOS Safari 14 release (< 14.4) don't support this.

Darn, you're right @KaichenWang. I forgot the support for gap outside a grid layout isn't as good. It won't be too long before this is viable, but I don't feel good about it as is.

Unfortunately the next simplest option I see (that doesn't change existing spacing under any other circumstances/conditions) involves including this new price--unit-price class so we can add margin only when the sale badge and unit price are displayed. Thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

If unit price is not displayed, wouldn't the sale badge still require margin around it?

Screen Shot 2021-08-17 at 4 31 09 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.

It should be receiving the 1rem right margin it was previously. When using the gap solution, I had prevented the margin-right of 1rem on the last price item, since gap was covering both the vertical and horizontal spacing. But that should be reverted now and that margin should always be there unless there's some configuration I'm not aware of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's a shame gap isn't well-supported enough at least for now. Regarding vertical spacing, I feel there should be margin above the badge when it wraps

Screen_Shot_2021-08-18_at_9_37_59_AM
Screen_Shot_2021-08-18_at_9_37_45_AM

Copy link
Contributor

@ludoboludo ludoboludo Aug 18, 2021

Choose a reason for hiding this comment

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

I think it looks good right now no ? 🤔 Or do you think there should be more spacing than there is on your second screenshot @KaichenWang ?

Also I wonder how accurate is that gap support chart on caniuse. When I test on Safari which seem to be version 14.1 on my laptop, it works 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Most likely yeah 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I'm fine with merging as is

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be late at the party! We can add a 0.5rem margin to help space out the items, it works well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paired with Meli and we found a compromise that will allow us to simplify this code and include the additional spacing Kai wants. Will create a follow up PR to address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up to this convo here #430

@ludoboludo ludoboludo mentioned this pull request Aug 17, 2021
5 tasks
@kmeleta kmeleta requested a review from ludoboludo August 18, 2021 14:27
@KaichenWang KaichenWang self-requested a review August 18, 2021 14:59
Copy link
Contributor

@KaichenWang KaichenWang left a comment

Choose a reason for hiding this comment

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

🚢

@kmeleta kmeleta merged commit 002b6b0 into main Aug 18, 2021
@kmeleta kmeleta deleted the product-card-price-polish branch August 18, 2021 15:01
@kmeleta kmeleta mentioned this pull request Aug 18, 2021
5 tasks
tairli added a commit to tairli/dawn that referenced this pull request Aug 22, 2021
commit 399626d
Author: tairli <tairli@yahoo.com>
Date:   Mon Aug 23 04:47:25 2021 +0930

    Update product-form.js

commit 330e6b4
Merge: 79467e4 7a2f34f
Author: tairli <tairli@yahoo.com>
Date:   Mon Aug 23 04:45:54 2021 +0930

    Merge remote-tracking branch 'upstream/main' into main

commit 7a2f34f
Author: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com>
Date:   Thu Aug 19 15:55:39 2021 -0400

    Add featured product section (Shopify#261)

commit 9789ae1
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 15:52:08 2021 -0400

    Update 2 translation files (Shopify#449)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit d83f16f
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 15:51:51 2021 -0400

    Update 7 translation files (Shopify#448)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit ce0e5ec
Author: Ludo <ludo.segura@shopify.com>
Date:   Thu Aug 19 14:46:05 2021 -0400

    Hide variant images setting (Shopify#158)

commit d3b44e7
Author: Ludo <ludo.segura@shopify.com>
Date:   Thu Aug 19 10:29:50 2021 -0400

    Update font style for Default preset (Shopify#435)

    * Update font style for Default preset

    * updated default in settings_schema

commit 2643bab
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 10:29:34 2021 -0400

    Update 15 translation files (Shopify#439)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit e9a2fd4
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Aug 19 10:02:16 2021 -0400

    Update 4 translation files (Shopify#440)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit 9ea0392
Author: Ludo <ludo.segura@shopify.com>
Date:   Wed Aug 18 17:19:16 2021 -0400

    404 continue shopping style (Shopify#419)

commit 6c2128a
Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
Date:   Wed Aug 18 15:14:33 2021 -0600

    Update price sale badge spacing (Shopify#430)

commit a6d25cf
Author: Kai <KaichenWang@users.noreply.github.com>
Date:   Wed Aug 18 11:13:22 2021 -0400

    Adjust global focus styles for inputs, selects and collection filter UI elements (Shopify#389)

    * Adjust global focus styles for inputs, selects and collection filter UI elements

    * Fix focus for filter drawer summary elements

    * Simplify focus for elements that are always "focus-visible" when in focus

    * Adjust focus style of search submit button

commit 002b6b0
Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com>
Date:   Wed Aug 18 09:01:47 2021 -0600

    Product card spacing and price polish (Shopify#407)

    * Adjust product card spacing and price styles

    * Fix unit price/sale badge spacing

    * Revert flex gap approach

commit 6fa46b7
Author: Ludo <ludo.segura@shopify.com>
Date:   Wed Aug 18 10:54:58 2021 -0400

    Update Preset default (Shopify#410)

commit 341e051
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Wed Aug 18 10:43:37 2021 -0400

    Update 10 translation files (Shopify#427)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit 79467e4
Merge: 42cad96 789c3d9
Author: tairli <tairli@yahoo.com>
Date:   Wed Aug 18 19:26:00 2021 +0930

    Merge branch 'Shopify:main' into main

commit 42cad96
Merge: b5ddb38 2ba2725
Author: tairli <tairli@yahoo.com>
Date:   Tue Aug 17 22:06:29 2021 +0930

    Merge branch 'Shopify:main' into main
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Adjust product card spacing and price styles

* Fix unit price/sale badge spacing

* Revert flex gap approach
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Product card spacing
5 participants