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

Jump to bottom

Add padding control to collection banner #1790

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+38 −15
Open

Add padding control to collection banner #1790

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
212b901
Add padding control to collection banner
kjellr Jun 14, 2022
85ea51c
Apply more specificity, conditionally apply styles.
kjellr Jun 17, 2022
d6beaa5
Revert "Apply more specificity, conditionally apply styles."
kjellr Jun 17, 2022
7fc5ceb
Remove margin, even out padding.
kjellr Jun 17, 2022
101e4b3
More opinionated padding rules.
kjellr Jul 1, 2022
be33dfe
Remove duplicate CSS rule.
kjellr Jul 5, 2022
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
14 changes: 8 additions & 6 deletions assets/component-collection-hero.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,6 @@
}

@media screen and (min-width: 750px) {
.collection-hero.collection-hero--with-image {
padding: calc(4rem + var(--page-width-margin)) 0
calc(4rem + var(--page-width-margin));
Comment on lines -13 to -14
Copy link
Contributor

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 of 0

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, that'll definitely look broken. 😕 Let's see what Melissa thinks too.

Copy link
Contributor

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 🙂

Copy link
Contributor Author

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.

overflow: hidden;
}

.collection-hero--with-image .collection-hero__inner {
padding-bottom: 0;
}
Expand All @@ -24,6 +18,14 @@
flex-basis: 100%;
}

.collection-hero__text-wrapper > *:first-child {
margin-top: 0;
}

.collection-hero__text-wrapper > *:last-child {
margin-bottom: 0;
}

@media screen and (min-width: 750px) {
.collection-hero {
padding: 0;
Expand Down
37 changes: 36 additions & 1 deletion sections/main-collection-banner.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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

Copy link
Contributor Author

@kjellr kjellr Jun 15, 2022 •
edited
Loading

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...

Copy link
Contributor

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.

Copy link
Contributor

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 👍🏼

Copy link
Contributor

@melissaperreault melissaperreault Jun 17, 2022 •
edited
Loading

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. 🤔

@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">
Expand Down Expand Up @@ -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": 24
},
{
"type": "range",
"id": "padding_bottom",
"min": 0,
"max": 100,
"step": 4,
"unit": "px",
"label": "t:sections.all.padding.padding_bottom",
"default": 24
}
]
}
Expand Down