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

Add global sections settings #891

Merged
merged 6 commits into from
Nov 16, 2021
Merged

Add global sections settings #891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
528ea67
Add section-wrapper/borders to applicable sections
kmeleta Nov 15, 2021
f22e290
Add section spacing
kmeleta Nov 15, 2021
64f7b65
Remove default border thickness.
kmeleta Nov 16, 2021
c8ee32b
Merge remote-tracking branch 'origin/all-global-settings' into add-gl…
kmeleta Nov 16, 2021
dba5e0c
Merge remote-tracking branch 'origin/all-global-settings' into add-gl…
kmeleta Nov 16, 2021
0700783
Fix collage spacing
kmeleta Nov 16, 2021
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
34 changes: 26 additions & 8 deletions assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,28 +213,28 @@ html.no-js .no-js-hidden {
}

.element-margin {
margin-top: 5rem;
margin-top: var(--sections-spacing);
}

.spaced-section {
margin-top: 5rem;
margin-top: var(--sections-spacing);
}

.spaced-section:last-child {
margin-bottom: 5rem;
margin-bottom: var(--sections-safe-spacing);
}

@media screen and (min-width: 750px) {
.element-margin {
margin-top: calc(5rem + var(--page-width-margin));
margin-top: var(--sections-spacing);
}

.spaced-section {
margin-top: calc(5rem + var(--page-width-margin));
margin-top: var(--sections-spacing);
}

.spaced-section:last-child {
margin-bottom: calc(5rem + var(--page-width-margin));
margin-bottom: var(--sections-spacing);
}
}

Expand Down Expand Up @@ -272,6 +272,24 @@ body,
}
}

.section-wrapper {
border-radius: var(--sections-radius);
border: var(--sections-border-width) solid rgba(var(--color-foreground), var(--sections-border-opacity));
overflow: hidden;
}

.section-wrapper.section-wrapper--fullwidth {
padding-left: 0;
padding-right: 0;
border-radius: 0;
border-left: 0;
border-right: 0;
}

.section-wrapper--padded {
padding: 2rem;
}

.grid-auto-flow {
display: grid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

grid-auto-flow: column;
Expand Down Expand Up @@ -849,7 +867,7 @@ summary::-webkit-details-marker {
justify-content: space-between;
align-items: flex-end;
gap: 1rem;
margin: 4rem 0 3rem;
margin: var(--sections-safe-spacing) 0 3rem;
flex-wrap: wrap;
}

Expand All @@ -873,7 +891,7 @@ summary::-webkit-details-marker {
@media screen and (min-width: 990px) {
.title,
.title-wrapper-with-link {
margin: 5rem 0 3rem;
margin: var(--sections-safe-spacing) 0 3rem;
}

.title--primary {
Expand Down
2 changes: 1 addition & 1 deletion assets/collage.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
}

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

Expand Down
6 changes: 3 additions & 3 deletions assets/component-image-with-text.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
.image-with-text {
margin-top: 5rem;
margin-top: var(--sections-spacing);
}

.image-with-text:not(.color-scheme-background-1) {
margin-bottom: 5rem;
margin-bottom: var(--sections-spacing);
}

@media screen and (min-width: 750px) {
.image-with-text {
margin-bottom: calc(5rem + var(--page-width-margin));
margin-bottom: var(--sections-spacing);
}
}

Expand Down
11 changes: 6 additions & 5 deletions assets/newsletter-section.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
.newsletter--narrow .newsletter__wrapper,
.newsletter:not(.newsletter--narrow) .newsletter__wrapper.color-background-1 {
margin-top: 5rem;
margin-bottom: 5rem;
margin-top: var(--sections-spacing);
margin-bottom: var(--sections-spacing);
}

.newsletter__wrapper:not(.color-background-1) {
.newsletter__wrapper:not(.color-background-1),
.newsletter__wrapper.color-background-1 {
padding-top: 5rem;
padding-bottom: 5rem;
}

@media screen and (min-width: 750px) {
.newsletter--narrow .newsletter__wrapper,
.newsletter:not(.newsletter--narrow) .newsletter__wrapper.color-background-1 {
margin-top: calc(5rem + var(--page-width-margin));
margin-bottom: calc(5rem + var(--page-width-margin));
margin-top: var(--sections-spacing);
margin-bottom: var(--sections-spacing);
}

.newsletter__wrapper:not(.color-background-1) {
Expand Down
9 changes: 4 additions & 5 deletions assets/section-multicolumn.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@

@media screen and (max-width: 749px) {
.multicolumn.no-heading.background-secondary {
padding-top: 5rem;
padding-top: var(--sections-safe-spacing);
}
}

@media screen and (min-width: 750px) {
.multicolumn.no-heading:not(.background-secondary) {
margin-top: calc(6rem + var(--page-width-margin));
margin-top: var(--sections-safe-spacing);
}
}

Expand Down Expand Up @@ -77,8 +77,7 @@

@media screen and (min-width: 750px) {
.multicolumn.background-secondary {
padding: calc(4rem + var(--page-width-margin)) 0
calc(5rem + var(--page-width-margin));
padding: var(--sections-safe-spacing) 0;
}
}

Expand All @@ -94,7 +93,7 @@

@media screen and (min-width: 750px) {
.multicolumn:not(.background-secondary) {
margin: calc(5rem + var(--page-width-margin)) 0;
margin: var(--sections-safe-spacing) 0;
}
}

Expand Down
8 changes: 4 additions & 4 deletions assets/section-rich-text.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@

.rich-text:not(.rich-text--full-width),
.rich-text--full-width.color-background-1 {
margin-top: 5rem;
margin-bottom: 5rem;
margin-top: var(--sections-spacing);
margin-bottom: var(--sections-spacing);
}

.rich-text:not(.color-background-1) {
Expand All @@ -37,8 +37,8 @@
@media screen and (min-width: 750px) {
.rich-text:not(.rich-text--full-width),
.rich-text--full-width.color-background-1 {
margin-top: calc(5rem + var(--page-width-margin));
margin-bottom: calc(5rem + var(--page-width-margin));
margin-top: var(--sections-spacing);
margin-bottom: var(--sections-spacing);
}

.rich-text:not(.color-background-1) {
Expand Down
4 changes: 2 additions & 2 deletions assets/video-section.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

@media screen and (min-width: 750px) {
.video-section.page-width {
margin-top: calc(5rem + var(--page-width-margin));
margin-bottom: calc(5rem + var(--page-width-margin));
margin-top: var(--sections-spacing);
margin-bottom: var(--sections-spacing);
}
}

Expand Down
45 changes: 45 additions & 0 deletions config/settings_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,51 @@
}
]
},
{
"name": "Sections",
"settings": [
{
"type": "range",
"id": "sections_spacing",
"min": 0,
"max": 80,
"step": 4,
"unit": "px",
"label": "Vertical spacing between sections",
"default": 40
},
{
"type": "range",
"id": "sections_border_opacity",
"min": 0,
"max": 100,
"step": 5,
"unit": "%",
"label": "Border opacity",
"default": 50
},
{
"type": "range",
"id": "sections_border_width",
"min": 0,
"max": 10,
"step": 1,
"unit": "px",
"label": "Border thickness",
"default": 0
},
{
"type": "range",
"id": "sections_radius",
"min": 0,
"max": 30,
"step": 3,
"unit": "px",
"label": "Corner radius",
"default": 0
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

]
},
{
"name": "Media",
"settings": [
Expand Down
10 changes: 9 additions & 1 deletion layout/theme.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,22 @@
--page-width: {{ settings.page_width | divided_by: 10 }}rem;
--page-width-margin: {% if settings.page_width == '1600' %}2{% else %}0{% endif %}rem;

--sections-spacing: {{ settings.sections_spacing }}px;
--sections-safe-spacing: {% if settings.sections_spacing < 15 %}15{% else %}{{settings.sections_spacing}}{% endif %}px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious as this was named safe spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

--sections-padding: 20px;
--sections-radius: {{ settings.sections_radius }}px;
--sections-border-width: {{ settings.sections_border_width }}px;
--sections-border-opacity: {{ settings.sections_border_opacity | divided_by: 100.0 }};
--sections-border-color: {{ settings.sections_border_color.red }}, {{ settings.sections_border_color.green }}, {{ settings.sections_border_color.blue }};

--popup-drawer-border-width: {{ settings.popup_drawer_border_width }}px;
--popup-drawer-border-opacity: {{ settings.popup_drawer_border_opacity | divided_by: 100.0 }};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

--popup-drawer-corner-radius: {{ settings.popup_drawer_corner_radius }}px;
--popup-drawer-shadow-opacity: {{ settings.popup_drawer_shadow_opacity | divided_by: 100.0 }};
--popup-drawer-shadow-horizontal-offset: {{ settings.popup_drawer_shadow_horizontal_offset }}px;
--popup-drawer-shadow-vertical-offset: {{ settings.popup_drawer_shadow_vertical_offset }}px;
--popup-drawer-shadow-blur-radius: {{ settings.popup_drawer_blur_radius }}px;

{% comment %} Grid styles {% endcomment %}
--grid-desktop-vertical-spacing: {{ settings.grid_desktop_vertical_spacing | divided_by: 10.0 }}rem;
--grid-desktop-horizontal-spacing: {{ settings.grid_desktop_horizontal_spacing | divided_by: 10.0 }}rem;
Expand Down
Loading