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

Product card spacing and price polish #407

Merged
merged 3 commits into from
Aug 18, 2021
Merged

Product card spacing and price polish #407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd228ba
Adjust product card spacing and price styles
kmeleta Aug 16, 2021
5f701b4
Fix unit price/sale badge spacing
kmeleta Aug 16, 2021
42da681
Revert flex gap approach
kmeleta Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions assets/component-card.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@
}

.card + .card-information {
margin-top: 1.2rem;
margin-top: 1.3rem;
}

@media screen and (min-width: 750px) {
.card + .card-information {
margin-top: 1.7rem;
}
}

.card.card--soft {
Expand Down Expand Up @@ -175,7 +181,7 @@
}

.card-information__wrapper > *:not(.visually-hidden:first-child) + * {
margin-top: 1.1rem;
margin-top: 0.7rem;
}

.card-information__wrapper .caption {
Expand Down
1 change: 1 addition & 0 deletions assets/component-price.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
.price--on-sale .price-item--regular {
text-decoration: line-through;
color: rgba(var(--color-foreground), 0.75);
font-size: 1.3rem;
}

.unit-price {
Expand Down
22 changes: 13 additions & 9 deletions assets/customer.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,15 +473,6 @@
}
}

.order tbody td:nth-of-type(3) dd:nth-of-type(2) {
font-size: 1.1rem;
letter-spacing: 0.07rem;
line-height: 1.2;
margin-top: 0.2rem;
text-transform: uppercase;
color: var(--color-foreground-70);
}

.order tfoot tr:last-of-type td,
.order tfoot tr:last-of-type th {
font-size: 2.2rem;
Expand Down Expand Up @@ -617,6 +608,19 @@
color: rgba(var(--color-foreground), 0.7);
}

.order .unit-price {
font-size: 1.1rem;
letter-spacing: 0.07rem;
line-height: 1.2;
margin-top: 0.2rem;
text-transform: uppercase;
color: rgba(var(--color-foreground), 0.7);
}

.order .regular-price {
font-size: 1.3rem;
}

/* Addresses */
.addresses li > button {
margin-left: 0.5rem;
Expand Down
12 changes: 12 additions & 0 deletions assets/section-main-product.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,14 @@ a.product__text {
align-items: flex-start;
}

.product .price--on-sale.price--unit-price {
margin-bottom: -0.5rem;
}

.product .price--on-sale.price--unit-price dl {
margin-bottom: 0.5rem;
}

.product .price--sold-out .price__badge-sold-out {
background: transparent;
color: rgb(var(--color-background));
Expand All @@ -276,6 +284,10 @@ a.product__text {
}

@media screen and (min-width: 750px) {
.product__info-container .price--on-sale .price-item--regular {
font-size: 1.6rem;
}

.product__info-container > *:first-child {
margin-top: 0;
}
Expand Down
3 changes: 2 additions & 1 deletion snippets/price.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

<dl>
{%- comment -%}
Explanation of description list:
Expand Down
6 changes: 3 additions & 3 deletions templates/customers/order.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 😅

<s>{{ line_item.original_price | money }}</s>
</dd>
<dt>
Expand All @@ -150,7 +150,7 @@
<dt>
<span class="visually-hidden">{{ 'products.product.price.unit_price' | t }}</span>
</dt>
<dd>
<dd class="unit-price">
<span>
{%- capture unit_price_separator -%}
<span aria-hidden="true">/</span><span class="visually-hidden">{{ 'accessibility.unit_price_separator' | t }}&nbsp;</span>
Expand Down Expand Up @@ -187,7 +187,7 @@
<dt>
<span class="visually-hidden">{{ 'products.product.price.regular_price' | t }}</span>
</dt>
<dd>
<dd class="regular-price">
<s>{{ line_item.original_line_price | money }}</s>
</dd>
<dt>
Expand Down