Additional padding and margin settings

[tyleralsbury]

Why are these changes introduced?

Fixes #1039.

What approach did you take?

• A continuation of the approach to the rest of the section padding, applied to more sections (particularly main sections as well as static sections)
• Added top margin setting to footer
• Added bottom margin setting to header

Other considerations

Demo links

• Store
• Editor

Checklist

• Followed theme code principles
• Linted with Theme Check
• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[ludoboludo]

I wonder if here we should have a setting as well or not. Maybe it's overkill... but a value a bit bigger I think would be nicer. Like the default 36px used for section padding.

[tyleralsbury]

This one is a bit weird because it's not its own section, so the setting is a bit different in terms of context. I can change the hardcoded value, but we'll need to think of how we want to handle a case like this where it's the not the padding of the full section - or we need to extract the comments part of the blog post into its own section for consistency.

[ludoboludo]

Yeah 🤔 At first I was going to say that it could reuse the value from the "section" but since it's a template it doesn't have it.

[tyleralsbury]

It wouldn't be particularly intuitive to use a section setting that works consistently everywhere else in a different way - I think the path forward is probably to turn the comments bit into its own separate section from the article itself and then apply the setting that way. Sort of similar to the way the cart items and cart footer are separate, for example.

[ludoboludo]

Yeah I think that makes sense 🤔

[ludoboludo]

Curious to know which ones we had as an offender here ?

[tyleralsbury]

This accounts for any case where .grid is the last element - looking through the code it seems like this is often the case. The only thing that it does cause, which might be an issue, is that if it's the last child of some parent element that has something else after it it might not be great.

I'm going to go through and see if that's the case anywhere. I notice that it does squish the margin between the cards in Featured Collection and the View all button.

[tyleralsbury]

Turns out there's a lot of inconsistencies with those types of buttons...

I've added some spacing for the one for Featured Collection cause it was wonky looking, but just looking at the remaining ones:

// Blog view all button

.blog__button {
  margin-top: 3rem;
}

@media screen and (min-width: 750px) {
  .blog__button {
    margin-top: 5rem;
  }
}

// Multicolumn

.multicolumn .button {
  margin-top: 1.5rem;
}

@media screen and (min-width: 750px) {
  .multicolumn .button {
    margin-top: 4rem;
  }
}

And both collection list and featured collection had just 2rem. I think changing them all to 2rem all the time would be good for consistency, and would potentially reduce CSS if we considered them to be a "button at the bottom of a section" component (naming is hard) and just used a class for all of them.

[KaichenWang]

We could look into why the grid component (which is used in a variety of scenarios) imposes a 2rem bottom margin in the first place. I'd be in favour of setting .grid to a default zero top and bottom margin and only add margin in cases where it's really needed. That would require testing each scenario, so probably out of scope for this PR.

[ludoboludo]

A couple things we could fix, brought up in slack:

• .newsletter:not(.newsletter--narrow) .newsletter__wrapper.color-background-1 is applying some extra margin we shouldn't need anymore due to the settings we offer.
• I think that you mention this in another PR/context but it seems like the section title margin is off sometimes. We have .title-wrapper-with-link which applies a top margin but I don't think we need it anymore. And we also have .title-wrapper--no-top-margin to remove that top margin... But im pretty sure you mentioned looking into it in a specific refactor issue for titles.

[tyleralsbury]

• I think that you mention this in another PR/context but it seems like the section title margin is off sometimes. We have .title-wrapper-with-link which applies a top margin but I don't think we need it anymore. And we also have .title-wrapper--no-top-margin to remove that top margin... But im pretty sure you mentioned looking into it in a specific refactor issue for titles.

Yeah, for now I'll just implement it the same was as before and then when I get a chance, I will refactor them all to just be .title and .title--with-link or something because there's no reason to have so many different modifiers at this stage.

[ludoboludo]

Alrighty, I think it's the last couple things I will mention 🤔

If we could edit the section title for both Multicolumn and Collapsible content that would make the spacing look good everywhere I believe. It's a matter of just adding the class .title-wrapper--no-top-margin:

• Multicolumn
• Collapsible content

[KaichenWang]

We could look into why the grid component (which is used in a variety of scenarios) imposes a 2rem bottom margin in the first place. I'd be in favour of setting .grid to a default zero top and bottom margin and only add margin in cases where it's really needed. That would require testing each scenario, so probably out of scope for this PR.

[tyleralsbury]

We could look into why the grid component (which is used in a variety of scenarios) imposes a 2rem bottom margin in the first place. I'd be in favour of setting .grid to a default zero top and bottom margin and only add margin in cases where it's really needed. That would require testing each scenario, so probably out of scope for this PR.

I looked at all of the scenarios where .grid was the last child and tested them to try and account for things. For now, this seems like a decent solution, but you're right that I think the long-term ideal solution is for the grid itself to not impose margins to begin with and for the context to dictate that.

It's sort of similar to the added complexity of .title and its myriad of modifiers - we've added things to a class that might be better served by handling it in-context.

[tyleralsbury]

Reviews got dismissed because of the end of file new line commit @ludoboludo @KaichenWang