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

[Cart] Add color picker on cart page and in general cart settings #3021

Merged
merged 11 commits into from
Oct 23, 2023
Merged

[Cart] Add color picker on cart page and in general cart settings #3021

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a9ee92b
rebase final final
ludoboludo Oct 12, 2023
c138200
use the update for the loading spinner
ludoboludo Oct 12, 2023
431ecdb
add color picker for the cart drawer and cart popup
ludoboludo Oct 13, 2023
c6bdca4
fix spacing issues on cart page
ludoboludo Oct 16, 2023
735ad7a
add gradient support
ludoboludo Oct 16, 2023
057e462
address review comments
ludoboludo Oct 19, 2023
c3f2bb5
adjust where the class is added. Fix gradient issue
ludoboludo Oct 19, 2023
32e5806
isolation needed for shadow
ludoboludo Oct 20, 2023
bddc582
add comment
ludoboludo Oct 20, 2023
edee634
Remove unnecessary isolate
ludoboludo Oct 23, 2023
ec4d283
use the isolate class instead
ludoboludo Oct 23, 2023
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
29 changes: 15 additions & 14 deletions assets/component-cart-items.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
cart-items .title-wrapper-with-link {
margin-top: 0;
}

.cart-items td,
.cart-items th {
padding: 0;
Expand Down Expand Up @@ -48,12 +52,12 @@
line-height: calc(1 + 0.4 / var(--font-body-scale));
}

.cart-item__details>* {
.cart-item__details > * {
margin: 0;
max-width: 30rem;
}

.cart-item__details>*+* {
.cart-item__details > * + * {
margin-top: 0.6rem;
}

Expand Down Expand Up @@ -83,7 +87,7 @@
text-decoration-thickness: 0.2rem;
}

.cart-item__price-wrapper>* {
.cart-item__price-wrapper > * {
display: block;
margin: 0;
padding: 0;
Expand Down Expand Up @@ -167,7 +171,7 @@ cart-remove-button .icon-remove {
}
}

.cart-item .loading__spinner:not(.hidden)~* {
.cart-item .loading__spinner:not(.hidden) ~ * {
visibility: hidden;
}

Expand All @@ -185,18 +189,18 @@ cart-remove-button .icon-remove {
order: 1;
}

.cart-item__error-text+svg {
.cart-item__error-text + svg {
flex-shrink: 0;
width: 1.2rem;
margin-right: 0.5rem;
margin-top: 0.1rem;
}

.cart-item__error-text:empty+svg {
.cart-item__error-text:empty + svg {
display: none;
}

.product-option+.product-option {
.product-option + .product-option {
margin-top: 0.4rem;
}

Expand All @@ -210,7 +214,6 @@ cart-remove-button .icon-remove {
}

@media screen and (max-width: 749px) {

.cart-items,
.cart-items thead,
.cart-items tbody {
Expand Down Expand Up @@ -276,7 +279,7 @@ cart-remove-button .icon-remove {
width: 50%;
}

.cart-items th+th {
.cart-items th + th {
padding-left: 4rem;
}

Expand All @@ -289,7 +292,7 @@ cart-remove-button .icon-remove {
display: table-row;
}

.cart-item>td+td {
.cart-item > td + td {
padding-left: 4rem;
}

Expand All @@ -305,7 +308,7 @@ cart-remove-button .icon-remove {
margin: 0.5rem 0 0 1.5rem;
}

.cart-item__price-wrapper>*:only-child:not(.cart-item__discounted-prices) {
.cart-item__price-wrapper > *:only-child:not(.cart-item__discounted-prices) {
margin-top: 1rem;
}

Expand All @@ -315,7 +318,6 @@ cart-remove-button .icon-remove {
}

@media screen and (min-width: 990px) {

.cart-item .cart-item__quantity,
.cart-items .cart-items__heading--wide {
padding-left: 6rem;
Expand All @@ -331,10 +333,9 @@ cart-remove-button .icon-remove {
}

@media screen and (min-width: 750px) {

.cart-items .cart-items__heading--quantity,
.cart-item .cart-item__quantity,
.cart-item__quantity--info quantity-popover>*,
.cart-item__quantity--info quantity-popover > *,
.no-js .cart-item .cart-item__quantity--info {
padding-left: 5rem;
}
Expand Down
6 changes: 4 additions & 2 deletions assets/component-cart.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,6 @@ cart-items {
top: 2.5rem;
}



.cart__note label {
display: flex;
align-items: flex-end;
Expand Down Expand Up @@ -170,6 +168,10 @@ cart-items {
margin: 0 auto;
}

.cart__dynamic-checkout-buttons:has(.dynamic-checkout__content:empty) {
margin: 0;
}

.cart__dynamic-checkout-buttons div[role='button'] {
border-radius: var(--buttons-radius-outset) !important;
}
Expand Down
6 changes: 6 additions & 0 deletions config/settings_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1448,6 +1448,12 @@
"label": "t:settings_schema.cart.settings.show_cart_note.label",
"default": false
},
{
"type": "color_scheme",
"id": "cart_color_scheme",
"label": "t:sections.all.colors.label",
"default": "background-1"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't entirely mind having the theme settings for the cart drawer and the cart page section settings separate. However..

  • I'm not certain how often the desire to differentiate the colors for each use case would come up. Probably not enough to warrant additional complexity.
  • The cart items and footer sections haven't historically had useful settings so a user may not naturally think to look in there for further style options.
  • The cart theme settings already contains settings that apply to the cart page so I don't think it's a big leap to use declare a shared color setting here as well

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do keep it this way, adding some helper text that clarifies the scope of this setting and/or where the cart page color scheme can be configured might be useful.

Copy link
Contributor

@eugenekasimov eugenekasimov Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • The cart items and footer sections haven't historically had useful settings so a user may not naturally think to look in there for further style options.

I have the same opinion on this, but it's definitely not a blocker.

{
"type": "header",
"content": "t:settings_schema.cart.settings.cart_drawer.header"
Expand Down
2 changes: 1 addition & 1 deletion sections/header.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@
</{% if section.settings.sticky_header_type != 'none' %}sticky-header{% else %}div{% endif %}>

{%- if settings.cart_type == "notification" -%}
{%- render 'cart-notification', color_scheme: section.settings.color_scheme, desktop_menu_type: section.settings.menu_type_desktop -%}
{%- render 'cart-notification', desktop_menu_type: section.settings.menu_type_desktop -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd also prefer the cart notification to continue inheriting from the header. Not to say the current implementation wouldn't be successful, I guess I just feel like there's a pretty decent chance of that being overlooked when customizing the cart color, being that the notification is not a persistent element.

And unless you really have a particular, intentional vision for it, I think the notification tends to look best matching the header where it spawns from. This would also be inline with the search dialog which also inherits its color from the header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with that take. I think it makes sense for it to inherit the color scheme of the header.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@melissaperreault melissaperreault Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah!... not an easy one!
The more I think about it is that we should have the color picker in theme settings only for the drawer because there are no other spaces where this setting would live, unless we move it to drawer category but that would be unexpected as you select the drawer cart type...

Another thing I am wondering is keeping color scheme setting together with their respective padding setting. This will make adjusting white space more naturally than trying to find where to adjust those spacings (oh, I need to adjust this space so I'll go in theme settings...). I know it's a tradeoff but I feel it's the right level of granularity considering the options we have. It could be interesting for a merchant to create some contrast between their cart table and the subtotal part. (Visual example 1) (Visual example 2)

My suggestions

  • Move the color scheme setting under the Cart drawer heading, under the Collection picker! ✨
  • Setting doesn't impact popup notification, no change. We can wait and learn from alternative use cases.

{%- endif -%}

{% javascript %}
Expand Down
14 changes: 12 additions & 2 deletions sections/main-cart-footer.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@
}
{%- endstyle -%}

<div class="page-width{% if cart == empty %} is-empty{% endif %}" id="main-cart-footer" data-id="{{ section.id }}">
<div>
<div
class="color-{{ section.settings.color_scheme }}{% if cart == empty %} is-empty{% endif %}"
id="main-cart-footer"
data-id="{{ section.id }}"
>
<div class="page-width">
<div class="cart__footer section-{{ section.id }}-padding">
{%- if settings.show_cart_note -%}
<cart-note class="cart__note field">
Expand Down Expand Up @@ -133,6 +137,12 @@
"name": "t:sections.main-cart-footer.name",
"class": "cart__footer-wrapper",
"settings": [
{
"type": "color_scheme",
"id": "color_scheme",
"label": "t:sections.all.colors.label",
"default": "background-1"
},
{
"type": "range",
"id": "padding_top",
Expand Down
Loading