-
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?
Conversation
Works as expected @kjellr. |
Yeah, that's my understanding. I'm pulling it from a shared set of existing strings. |
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.
Looks good 👍 🚀
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.
Actually... now Im thinking about it. I feel like it might be nice to go further and tweak the margins of the header as well 🤔
It would be nice if the experience was consistent across templates/pages. If I go to other pages for example, here is the experience: video.
Basically on some templates/pages you can have a padding top of 0
that results in having the header/section almost touching the navigation but in some others like blogs, collection banner, etc, you can't get it as there is a margin top already applied.
Maybe something to tackle as a follow up/parity across pages.
Good call — the margin values there can be really confusing for users. Seems like the sort of thing that would be good to followup on globally. |
32ab6ed
to
212b901
Compare
.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; | ||
} | ||
} |
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.
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 (max-width: 749px
) but not on desktop:
See video
collection-banner--section-padding-setting.mp4
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.
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:
- If the image is off, the default padding is
0px
. - If the image is active, the default padding is
40px
(4rem
) - If the user has modified the padding slider, use their new value regardless of the collection image setting.
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 comment
The 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 (times: 0.75
):
dawn/sections/rich-text.liquid
Lines 7 to 19 in 8d0e9fb
{%- 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; | |
} | |
} | |
{%- endstyle -%} |
… 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
If the image is active, the default padding is 40px (4rem) - If the user has modified the padding slider, use their new value regardless of the collection image setting.
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. 🤔
Observation related to the marginsOn
I didn't test mobile neither the collection image setting yet but wanted to give this perspective before moving forward. |
"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 comment
The reason will be displayed to describe this comment to others. Learn more.
"default": 0 | |
"default": 24 |
"step": 4, | ||
"unit": "px", | ||
"label": "t:sections.all.padding.padding_top", | ||
"default": 0 |
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.
"default": 0 | |
"default": 24 |
This reverts commit 85ea51c.
Yeah, we can re-create those margins with padding, but the issue remains that the spacing is different when the image is active vs. when it isn't. So even with this approach, I think we'd have to end up making the current spacing values different, thus messing with the way folks' sites appear today. 🤔 An observation from a newcomer: It's weird that the schema for these controls is so static! This could be easily solved by putting a simple conditional in there for the default. Or conditionally showing the controls based on the value of another setting. This is something I expected would be easy, but it doesn't appear that it's possible. 🤨 |
We'll inevitably need to do this if we want to implement it the same way it is across other sections. The collection banner is a though one because it was already opinionated. This has been one of our initial principle to guide the flexibility with stakeholder feedback. What you see is what you get and avoid magic as much as you can. This is to align with being predictable, learnable and flexible. I am not sold on the single solution either for solving for when the image is active or not, but there is something to look into. We do have some exceptions to the rule like when we have shadows in sliders on smaller screen. To avoid cutting off the blur we increase the space between the cards and the slider control. We have been referring it to systematical change to not have visual broken features. Another potential solution for predictability that we could get is offering a
Yeah, not possible right now. @tyleralsbury could expand more on the technicalities! |
Oh wow, ok! I got back in town! 😅 |
TL;DR for this one - there is a strict limitation that exists within the Shopify platform where settings do not have an awareness of other settings. So if you've got settings with This means that there are a number of things that could be cool that cannot be done. One such example is conditional setting visibility, as you're describing. Another is the capacity to override a global setting with a section or block level setting. Feel free to reach out @kjellr if you want to sync about this, I can give more details. |
Thanks for the context!
I've pushed this to the PR. Give the demo a try and let me know what you think. Here are some screenshots so we can see how the default appearance would change with
|
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.
It's looking good. I think we might benefit from reworking some of the padding on mobile maybe as right now it's using a lot of whitespace.
There is some padding we could potentially remove/reduce. Video to explain it
Thanks, @ludoboludo. I've pushed some more opinionated padding rules that clean this all up. Here's a quick demo: 01-56-qxns2-1t1td.mp4Give it a spin and let me know what you folks think. I'm a little concerned that it'll be a more noticeable change for Dawn users — I'm not sure it'll be a welcome one for everyone? Not sure how careful we are about that usually, so @melissaperreault I'd love some perspective here. I'm also not sure if it's weird to have the default bottom padding value be 0px like this. We could give it a different value, but then we'd have to modify the default top padding value of the Product Grid to compensate. 😅 |
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.
It's looking good I think. I left a couple comments.
padding: calc(4rem + var(--page-width-margin)) 0 | ||
calc(4rem + var(--page-width-margin)); |
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.
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.
Here's a set of screenshots showing the visual impact of the spacing updates noted above: Heading + DescriptionDesktop
Mobile
Heading + Description + ImageDesktop
Mobile
Here's the impact if a user has removed the top padding from the subsequent product grid block: Heading + DescriptionDesktop
Mobile
Heading + Description + ImageDesktop
Mobile
|
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 think it's looking good 👌 Thanks for the changes and the screen captures to show/explain the impact of the changes 🙂
Hey team, The probot-CLA was recently deprecated and replaced with the shopify-cla action. To successfully use the new shopify-cla status check, this PR will be closed and reopened. Thanks and sorry for the minor inconvenience. Message me if you have any questions 😄 Please refer to the following for more information: |
PR Summary:
Why are these changes introduced?
Greater design flexibility.
What approach did you take?
Mirrors the approach taken for adding padding controls to other sections.
Other considerations
This one starts with the values at zero, since we currently don't include any padding here.
Testing steps/scenarios
Checklist