Add global sections settings

[kmeleta]

Why are these changes introduced?

Added section settings for borders and vertical spacing.

What approach did you take?

Border styles and corner radius have been tied to a section-wrapper classes which have been added where needed to each applicable section. Some area that can receive borders are best left with some amount of padding (value tbd) and I've applied this via section-wrapper--padded. Additionally, some sections have fullwidth display modes. Left and right borders and well as corner radius have been excluded in these cases.

Using this approach seemed like a good idea when there were more settings involved, but maybe now with just borders the styles can be applied directly to existing elements in each section.

Borders have been excluded from...

• Featured collection
• Collection list
• Multicolumn

Vertical spacing is applied to global spaced-section type classes, but also to section specific stylesheets which override those global classes. I've also included the concept of "safe spacing" in some uses of these spacing variables to ensure some amount of spacing is used in sections where zero spacing isn't ideal. How exactly this spacing interacts with heading margins is tbd.

Other considerations

Border styles have been applied where we most likely want them. Some sections have been excluded from this until local overrides are available. UX is still considering the details of this and more sections may be included or excluded.

To better facilitate vertical spacing between sections, the methods by which each section currently manages this space should probably be completely reworked. Many sections have their own margins and paddings applied to different elements and each of these can change based on other settings specific to each section. Globally controlling all these exceptions is sure to lead to bugs and inconsistencies.

Demo links

• Editor

Checklist

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

[tyleralsbury]

Looks good from testing. Will need to take a more in-depth look at the code changes; I see a lot of refactoring.

One thing to think about is if/how these settings should affect main sections for pages like the main product section or collection grid. Currently this is only affecting sections that are "sections everywhere" sections when it comes to the borders.

cc @melissaperreault

[kmeleta]

Thanks @tyleralsbury

I see a lot of refactoring.

If you mean in the liquid, it's mostly just adding a new div parent to some sections to contain the border styles. Does look kinda scary in diffs. Otherwise I'd honestly like to refactor more around the how spaced-section margins are applied and subsequently overridden by many sections.

Currently this is only affecting sections that are "sections everywhere" sections when it comes to the borders.

Much of the discussion to this point has been around the re-usable sections so the others got overlooked. I can add the others shortly (in a followup if that's best)

[tyleralsbury]

Much of the discussion to this point has been around the re-usable sections so the others got overlooked. I can add the others shortly (in a followup if that's best)

I'm not sure what the expectation is for that, so it'd be a good idea to figure it out with @melissaperreault before we decide anything. In a follow-up for sure.

[sofiamatulis]

Looks good 👍 Added a few comments

[sofiamatulis]

I know we arent requesting translations yet, but what do you think of already adding the content to the en file?

[kmeleta]

This section still has questions around it and is sure to change. I think it's best to minimize the files touched until we're closer to nailing it down.

[sofiamatulis]

curious as this was named safe spacing

[kmeleta]

This is a concept to prevent this issue. Some sections can bump up against each other with 0px spacing, but others (ones with headings like this) should always have some spacing (tbd). I want to take time to align how sections/headings are applying margins so this could become less necessary.

[sofiamatulis]

Curious as to why the media for images is being changed on this PR

[kmeleta]

It shouldn't be, I think the the diffs just look weird because I added a div that encompasses all this. You'll notice the line being compared to this is a different part of the file (line 68 vs 87).

[tyleralsbury]

You can hide whitespace changes to make the diffs clearer, I think. It might help to solve some of these weird looking diffs.

[kmeleta]

Ah right, you can do that. Much better.

[sofiamatulis]

Same here

[sofiamatulis]

Is this supposed to be part of this PR?

[kmeleta]

Should just be whitespace in the diff.

[sofiamatulis]

Is this supposed to be part of this PR?

[kmeleta]

Should just be whitespace in the diff.

[sofiamatulis]

I think we should add an info here since this doesnt affect all borders. 🤔 It took me a bit to realize why it wasnt applying to all borders

cc @melissaperreault
https://screenshot.click/16-28-ak5vb-t961b.png

[sofiamatulis]

I think well need to re-visit everywhere where we were calculating the vertical spacing like

  .background-secondary {
    padding: calc(6rem + var(--page-width-margin)) 0
      calc(5rem + var(--page-width-margin));
  }
}

in base.css

Or where we add negative margin.

[kmeleta]

Don't quite know what to do with the background secondary cases yet. Will need to follow up with design after merge.

[sofiamatulis]

We will need to re-visit everywhere where we have negative margin

For instance in collage:

  .collage-section + .collage-section .no-heading {
    margin-top: calc(-4rem - var(--page-width-margin));
  }

this is happening: https://screenshot.click/16-36-6uzmu-j7ykj.png

[kmeleta]

I tried to fix the collage case so we can see that that use case better. But again the code involved here really needs to be cleaned up once the direction is settled on.

[sofiamatulis]

The negative margin and calculated margin scenarios can be addressed in a follow up 👍

For instance: collage + collage should have very little spacing
https://screenshot.click/16-29-i8qpy-1fkca.png