-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add padding control to collection banner #1790
base: main
Are you sure you want to change the base?
Changes from all commits
212b901
85ea51c
d6beaa5
7fc5ceb
101e4b3
be33dfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,14 +2,25 @@ | |||||||||||||||||||||||||||
{{ 'component-collection-hero.css' | asset_url | stylesheet_tag }} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
{%- style -%} | ||||||||||||||||||||||||||||
.section-{{ section.id }}-padding { | ||||||||||||||||||||||||||||
padding-top: {{ section.settings.padding_top | times: 0.75 | round: 0 }}px; | ||||||||||||||||||||||||||||
padding-bottom: {{ section.settings.padding_bottom | times: 0.75 | round: 0 }}px; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
@media screen and (min-width: 750px) { | ||||||||||||||||||||||||||||
.section-{{ section.id }}-padding { | ||||||||||||||||||||||||||||
padding-top: {{ section.settings.padding_top }}px; | ||||||||||||||||||||||||||||
padding-bottom: {{ section.settings.padding_bottom }}px; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
+5
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is looking good, thanks @kjellr I do have a question (and there's a good chance I'm missing some context on whether this was intentional) -- when I enable Collection banner > Show collection image, the section padding works on mobile ( See videocollection-banner--section-padding-setting.mp4There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, @andrewetchen! This is not intended. What's happening is that the default padding for the Collection Banner changes depending on whether "Show collection image" is active. I didn't realize that originally. 😩 I'm not sure the best way to handle this. The ideal behavior would be:
As far as I can tell though, I can't modify the setting's default depending on whether or not the image is turned on. Maybe I'm missing something though? I imagine we've run into this sort of thing elsewhere... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey Kjell, I'll re-share what I sent you on Slack: On the topic of the slider control value not matching the “actual”/computed padding… I don’t think this will be much of a problem since the slider control will always show 25% more than the computed padding because of this ( dawn/sections/rich-text.liquid Lines 7 to 19 in 8d0e9fb
… We can probably come up with a solution using a combination of the solutions we shared earlier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To reduce the noise in this PR, I created a draft PR (with store editor link) for the above: #1808 Feel free to comment on the draft PR 👍🏼 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Since we don't have the control to know if some collection pages will have or not an image displayed, the padding settings value need to map 1:1 what you have on the screen. I made a comment below about that. This means that even if the padding is set to 0, when the image is active, the preview will lie. It will lie because the settings are not conditionally changing with the choices you make. I think we will have no choice to lie. (I am still forming an opinion and don't see another solution right now) We need to consider also that these settings applies to ALL Collections regardless if they have images or not, a collection description or not. Potential solution Could we only let the padding increase as the ranges are exceeding 40px if the image is active? This way merchants could benefit from it depending on their reality? It's kind of unexpected but I don't think we have a quick solution for now. Collections without images would still have their preference set and it could expand as they go beyond the default 40px padding when images are around. 🤔 |
||||||||||||||||||||||||||||
@media screen and (max-width: 749px) { | ||||||||||||||||||||||||||||
.collection-hero--with-image .collection-hero__inner { | ||||||||||||||||||||||||||||
padding-bottom: calc({{ settings.media_shadow_vertical_offset | at_least: 0 }}px + 2rem); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
{%- endstyle -%} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
<div class="collection-hero{% if section.settings.show_collection_image and collection.image %} collection-hero--with-image{% endif %} color-{{ section.settings.color_scheme }} gradient"> | ||||||||||||||||||||||||||||
<div class="collection-hero{% if section.settings.show_collection_image and collection.image %} collection-hero--with-image{% endif %} color-{{ section.settings.color_scheme }} gradient section-{{ section.id }}-padding"> | ||||||||||||||||||||||||||||
<div class="collection-hero__inner page-width"> | ||||||||||||||||||||||||||||
<div class="collection-hero__text-wrapper"> | ||||||||||||||||||||||||||||
<h1 class="collection-hero__title"> | ||||||||||||||||||||||||||||
|
@@ -92,6 +103,30 @@ | |||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||
"default": "background-1", | ||||||||||||||||||||||||||||
"label": "t:sections.all.colors.label" | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
"type": "header", | ||||||||||||||||||||||||||||
"content": "t:sections.all.padding.section_padding_heading" | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
"type": "range", | ||||||||||||||||||||||||||||
"id": "padding_top", | ||||||||||||||||||||||||||||
"min": 0, | ||||||||||||||||||||||||||||
"max": 100, | ||||||||||||||||||||||||||||
"step": 4, | ||||||||||||||||||||||||||||
"unit": "px", | ||||||||||||||||||||||||||||
"label": "t:sections.all.padding.padding_top", | ||||||||||||||||||||||||||||
"default": 36 | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
"type": "range", | ||||||||||||||||||||||||||||
"id": "padding_bottom", | ||||||||||||||||||||||||||||
"min": 0, | ||||||||||||||||||||||||||||
"max": 100, | ||||||||||||||||||||||||||||
"step": 4, | ||||||||||||||||||||||||||||
"unit": "px", | ||||||||||||||||||||||||||||
"label": "t:sections.all.padding.padding_bottom", | ||||||||||||||||||||||||||||
"default": 0 | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, and we might have mentioned that already but there could be a visual impact for users who had set their product grid with a
padding-top
of0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that'll definitely look broken. 😕 Let's see what Melissa thinks too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you maybe bring up those use cases in which a visual impact will happen in the PR summary before the gif you have right now ?
This way when doing release notes, it can be caught right away and mentioned in there 👍
Those notes can be useful to merchants to be aware about what will need attention if they decide to upgrade manually 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've drafted a summary in the PR description above, and also included screenshots below.