Add padding control to collection banner #1790

kjellr · 2022-06-14T13:29:59Z

PR Summary:

Adds padding controls to the collection banner.
Trims down spacing around the collection banner to be more balanced. This may result in unexpected behavior if the merchant has removed the top padding from the subsequent block on their Collection template.

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

Open the test store
Try adjusting the new padding control on the Collection Banner.

Checklist

Added PR summary for release notes
Notified the help.shopify.com team about updates to theme settings (current contact: Kimli)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

allylane · 2022-06-14T14:15:50Z

Works as expected @kjellr.
Nice one.
Do you not need to update translations because it's under the "all" area in the schema?

kjellr · 2022-06-14T17:00:39Z

Do you not need to update translations because it's under the "all" area in the schema?

Yeah, that's my understanding. I'm pulling it from a shared set of existing strings.

ludoboludo

Looks good 👍 🚀

ludoboludo

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.

kjellr · 2022-06-15T16:51:50Z

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.

andrewetchen · 2022-06-15T17:39:57Z

sections/main-collection-banner.liquid

+  .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;
+    }
+  }


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

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

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.

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

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

melissaperreault · 2022-06-17T02:48:55Z

Observation related to the margins

On Rich text when both margins are set to 0, not extra space are interfering with the layout so when we chose a different color scheme you can see the limit touching the bottom or top

I wonder if we can clean it up and have some of the rules like applied on Rich text. Depending on the reordering of the blocks, you'll always get the margin applied between the elements, never on the first or the last to maintain predictability in how the setting is set and is previewed in the editor. (See below, I actually don't understand the rule equation but thought it was relevant.) Right now on the Collection banner we still see some margins even when the values are set to 0.

.rich-text__blocks > * {
margin-top: 0;
 margin-bottom: 0;
}

.rich-text__blocks > * + * {
margin-top: 2rem;
}

.rich-text__blocks > * + a {
margin-top: 3rem;
}

Video reference of me testing the right default values by introducing these settings to collection banner. This is only mapping out the reference for when the collection banner image is not selected.
- Video reference of testing out my assumptions to set the new default to their right expected value once we remove unwanted margins. The result gives me 24px for top and 20 for bottom. I made the suggestions where needed.

I didn't test mobile neither the collection image setting yet but wanted to give this perspective before moving forward.

melissaperreault · 2022-06-17T02:47:05Z

sections/main-collection-banner.liquid

+      "step": 4,
+      "unit": "px",
+      "label": "t:sections.all.padding.padding_bottom",
+      "default": 0


Suggested change

"default": 0

"default": 24

melissaperreault · 2022-06-17T02:47:09Z

sections/main-collection-banner.liquid

+      "step": 4,
+      "unit": "px",
+      "label": "t:sections.all.padding.padding_top",
+      "default": 0


Suggested change

"default": 0

"default": 24

This reverts commit 85ea51c.

kjellr · 2022-06-17T12:19:05Z

Right now on the Collection banner we still see some margins even when the values are set to 0.

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

melissaperreault · 2022-06-17T14:54:08Z

I think we'd have to end up making the current spacing values different, thus messing with the way folks' sites appear today.

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 Image padding range setting type under the Collection banner section and set it to 40px by default so that it matches what merchants currently have and it adds up to the section padding if it exists. We do this on cards to space out the media from the container. The cons would be that the banner could be taller by default for merchants doing manual updates because of the section padding default. Nothing that could not be adjusted with other padding settings from the product grid. This is something we can declare in the release notes as a visual change.

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.

Yeah, not possible right now. @tyleralsbury could expand more on the technicalities!

melissaperreault · 2022-06-17T17:38:01Z

Oh wow, ok! I got back in town! 😅
I think I got stopped at the fact that we would need to keep the 40px when image is active which we are not attached to. We could live with offering the default value and have it even for top and bottom. In this example, I've set it to padding: 2.4rem 0; by default, this would work well for both when the image is there or not.) Default value to be discussed if you feel 24px is not the right choice.

tyleralsbury · 2022-06-17T17:54:25Z

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.

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 id a and b, they have no awareness of each others' existence.

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.

kjellr · 2022-06-17T19:15:33Z

Thanks for the context!

I think I got stopped at the fact that we would need to keep the 40px when image is active which we are not attached to. We could live with offering the default value and have it even for top and bottom. In this example, I've set it to padding: 2.4rem 0; by default, this would work well for both when the image is there or not.) Default value to be discussed if you feel 24px is not the right choice.

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 24px of standardized padding:

Before	After

Before	After

ludoboludo

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

kjellr · 2022-07-01T18:02:25Z

Thanks, @ludoboludo. I've pushed some more opinionated padding rules that clean this all up. Here's a quick demo:

01-56-qxns2-1t1td.mp4

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

ludoboludo

It's looking good I think. I left a couple comments.

assets/component-collection-hero.css

ludoboludo · 2022-07-04T18:03:28Z

assets/component-collection-hero.css

-    padding: calc(4rem + var(--page-width-margin)) 0
-      calc(4rem + var(--page-width-margin));


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

Yeah, that'll definitely look broken. 😕 Let's see what Melissa thinks too.

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 🙂

Thanks. I've drafted a summary in the PR description above, and also included screenshots below.

kjellr · 2022-07-08T19:25:24Z

Here's a set of screenshots showing the visual impact of the spacing updates noted above:

Heading + Description

Desktop

Before	After

Mobile

Before	After

Heading + Description + Image

Desktop

Before	After

Mobile

Before	After

Here's the impact if a user has removed the top padding from the subsequent product grid block:

Heading + Description

Desktop

Before	After

Mobile

Before	After

Heading + Description + Image

Desktop

Before	After

Mobile

Before	After

ludoboludo

I think it's looking good 👌 Thanks for the changes and the screen captures to show/explain the impact of the changes 🙂

andrewetchen · 2022-07-27T01:11:59Z

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:

Add padding control to collection banner

212b901

kjellr added the Category: Enhancement New feature or request label Jun 14, 2022

kjellr self-assigned this Jun 14, 2022

ghost added the cla-needed label Jun 14, 2022

allylane requested a review from andrewetchen June 14, 2022 14:11

ludoboludo previously approved these changes Jun 15, 2022

View reviewed changes

ludoboludo reviewed Jun 15, 2022

View reviewed changes

martinamarien dismissed ludoboludo’s stale review via 212b901 June 15, 2022 17:17

martinamarien force-pushed the try/collection-banner-padding branch from 32ab6ed to 212b901 Compare June 15, 2022 17:17

andrewetchen reviewed Jun 15, 2022

View reviewed changes

andrewetchen mentioned this pull request Jun 16, 2022

Add padding to collection banner section #1808

Closed

melissaperreault reviewed Jun 17, 2022

View reviewed changes

Apply more specificity, conditionally apply styles.

85ea51c

ghost removed the cla-needed label Jun 17, 2022

Revert "Apply more specificity, conditionally apply styles."

d6beaa5

This reverts commit 85ea51c.

Remove margin, even out padding.

7fc5ceb

ludoboludo reviewed Jun 27, 2022

View reviewed changes

More opinionated padding rules.

101e4b3

ludoboludo reviewed Jul 4, 2022

View reviewed changes

Remove duplicate CSS rule.

be33dfe

ludoboludo approved these changes Jul 11, 2022

View reviewed changes

KaichenWang added the By: Team label Jul 18, 2022

andrewetchen closed this Jul 27, 2022

andrewetchen reopened this Jul 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add padding control to collection banner #1790

Add padding control to collection banner #1790

kjellr commented Jun 14, 2022 •

edited

Loading

allylane commented Jun 14, 2022

kjellr commented Jun 14, 2022

ludoboludo left a comment

ludoboludo left a comment

kjellr commented Jun 15, 2022

andrewetchen Jun 15, 2022

kjellr Jun 15, 2022 •

edited

Loading

andrewetchen Jun 16, 2022

andrewetchen Jun 16, 2022

melissaperreault Jun 17, 2022 •

edited

Loading

melissaperreault commented Jun 17, 2022 •

edited

Loading

melissaperreault Jun 17, 2022 •

edited

Loading

melissaperreault Jun 17, 2022

kjellr commented Jun 17, 2022

melissaperreault commented Jun 17, 2022 •

edited

Loading

melissaperreault commented Jun 17, 2022

tyleralsbury commented Jun 17, 2022

kjellr commented Jun 17, 2022

ludoboludo left a comment

kjellr commented Jul 1, 2022

ludoboludo left a comment

ludoboludo Jul 4, 2022

kjellr Jul 5, 2022

ludoboludo Jul 5, 2022

kjellr Jul 8, 2022

kjellr commented Jul 8, 2022

ludoboludo left a comment

andrewetchen commented Jul 27, 2022 •

edited

Loading

	{%- 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 -%}

		padding: calc(4rem + var(--page-width-margin)) 0
		calc(4rem + var(--page-width-margin));

Add padding control to collection banner #1790

Are you sure you want to change the base?

Add padding control to collection banner #1790

Conversation

kjellr commented Jun 14, 2022 • edited Loading

allylane commented Jun 14, 2022

kjellr commented Jun 14, 2022

ludoboludo left a comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

kjellr commented Jun 15, 2022

Choose a reason for hiding this comment

kjellr Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

melissaperreault commented Jun 17, 2022 • edited Loading

Observation related to the margins

melissaperreault Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjellr commented Jun 17, 2022

melissaperreault commented Jun 17, 2022 • edited Loading

melissaperreault commented Jun 17, 2022

tyleralsbury commented Jun 17, 2022

kjellr commented Jun 17, 2022

ludoboludo left a comment

Choose a reason for hiding this comment

kjellr commented Jul 1, 2022

ludoboludo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjellr commented Jul 8, 2022

Heading + Description

Heading + Description + Image

Heading + Description

Heading + Description + Image

ludoboludo left a comment

Choose a reason for hiding this comment

andrewetchen commented Jul 27, 2022 • edited Loading

kjellr commented Jun 14, 2022 •

edited

Loading

kjellr Jun 15, 2022 •

edited

Loading

melissaperreault Jun 17, 2022 •

edited

Loading

melissaperreault commented Jun 17, 2022 •

edited

Loading

melissaperreault Jun 17, 2022 •

edited

Loading

melissaperreault commented Jun 17, 2022 •

edited

Loading

andrewetchen commented Jul 27, 2022 •

edited

Loading