-
Notifications
You must be signed in to change notification settings - Fork 82
Refactor spacing to use CSS custom properties (#982) #1114
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
This will address some of what's in #1099 (comment) too. |
Refactor spacing system to use CSS custom properties. This is the third PR in a refactor of how we use CSS variables (#982), following the typography (#1107) and color refactors. Introduces --theme-spacing-between-block and --theme-spacing-between-inline for responsive layout spacing while retaining Sass $spacing-* variables for component-internal spacing. Modernization: Default to CSS vars for layout spacing (block and inline) Responsive breakpoints defined in root/_spacing.scss Updated components to use new spacing vars: containers, card, callout, split, breadcrumb, navigation, newsletter-form, section-heading, card-layout, multi-column Reorganization: Removed includes/_themes-sass.scss (no longer needed) Removed bidi-grid-spacing() mixin Removed deprecated grid-gap from stylelint ignored shorthands Simplification: Simplified split component (removed pop and overflow options in separate PR) Simplified card-layout and multi-column templates Reduced newsletter-form complexity Documentation: Added section-heading mzp-t-section-heading-nospace theme class Added spacing migration guide to docs/02-usage/migration.md Updated CHANGELOG with spacing system changes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
d77602f to
f566a57
Compare
| margin: 0 auto; | ||
| max-width: $content-max; | ||
| padding: 0 $h-grid-xs; | ||
| padding: 0 $layout-xs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❄️ 🤷♀️
| --h-grid-xl: 80px; | ||
| @media #{tokens.$mq-xl} { | ||
| --theme-spacing-between-block: #{tokens.$layout-xl}; // 96px | ||
| --theme-spacing-between-inline: 80px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❄️ 🤷♀️
|
Can I remove floats from hero layout? Edit: No, there's no grid based fallback so I'm going to consider this one out of scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the spacing system to use CSS custom properties (--theme-spacing-between-block and --theme-spacing-between-inline) for responsive layout spacing, following previous typography and color refactors. It modernizes the codebase by removing legacy fallbacks and deprecated variables while simplifying component implementations.
Changes:
- Introduced new CSS custom properties for spacing that are responsive across breakpoints
- Renamed card size classes (
mzp-c-card-extra-small→mzp-c-card-small) and removedmzp-c-card-mediumas the default - Removed legacy Sass spacing variables, float-based fallbacks, and deprecated
grid-gapshorthand
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| theme/assets/sass/components/_grid-list.scss | Updated grid-gap to standard gap property |
| theme/assets/sass/components/_color.scss | Updated grid-gap to standard gap property |
| docs/02-usage/migration.md | Added migration guide for card component changes |
| components/section-heading/readme.md | Documented new mzp-t-section-heading-nospace theme class |
| components/navigation/02-menu/readme.md | Updated reference from Extra Small Card to Small Card |
| components/navigation/02-menu/menu.html | Updated card class from mzp-c-card-extra-small to mzp-c-card-small |
| components/layout/03-card-layout/card-layout.config.yml | Updated card size recommendations in layout documentation |
| components/layout/03-card-layout/03-card-layout--quarter.html | Updated card renders to use @card--small instead of @card--extra-small |
| components/card/readme.md | Updated card size documentation and image size recommendations |
| components/card/card.config.yml | Removed extra-small variant, updated medium variant to be default |
| components/_preview-card.html | Added new preview template for card components |
| components/_preview-card-dark.html | Added new dark theme preview template for card components |
| assets/sass/protocol/templates/_multi-column.scss | Migrated to new spacing CSS variables and removed legacy fallbacks |
| assets/sass/protocol/templates/_card-layout.scss | Migrated to new spacing CSS variables and removed max-width restrictions |
| assets/sass/protocol/root/_spacing.scss | Replaced legacy grid variables with new theme spacing variables |
| assets/sass/protocol/includes/mixins/_utils.scss | Removed unused import of themes-sass |
| assets/sass/protocol/includes/_themes-sass.scss | Deleted file containing legacy Sass spacing variables |
| assets/sass/protocol/includes/_lib.scss | Removed forward of deleted themes-sass module |
| assets/sass/protocol/components/_split.scss | Migrated to new spacing variables and removed float-based fallbacks |
| assets/sass/protocol/components/_section-heading.scss | Migrated to new spacing variables and added nospace theme class |
| assets/sass/protocol/components/_newsletter-form.scss | Migrated to new spacing variables and removed float-based fallbacks |
| assets/sass/protocol/components/_navigation.scss | Simplified spacing with new CSS variables |
| assets/sass/protocol/components/_card.scss | Renamed extra-small to small, removed medium class, removed max-width restrictions |
| assets/sass/protocol/components/_callout.scss | Migrated to new spacing variables and removed float-based fallbacks |
| assets/sass/protocol/components/_breadcrumb.scss | Migrated to new spacing variables |
| assets/sass/protocol/base/elements/_containers.scss | Migrated to new spacing variables and removed legacy fallbacks |
| assets/sass/protocol/base/elements/_common.scss | Migrated hr element spacing to new CSS variables |
| CHANGELOG.md | Documented spacing system changes and card component updates |
| .stylelintrc.json | Removed deprecated grid-gap from ignored shorthands list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
maureenlholland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+wc 💫
Non-blocking stuff, doesn't affect actual styles
It's unclear from comments whether we removed the float fallback in Split component
Card demo/docs still has places where it assumes small is default
| padding: var(--theme-spacing-between-block) var(--theme-spacing-between-inline); | ||
| position: relative; | ||
|
|
||
| @media #{$mq-md} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 🧹 🧹
|
|
||
| .mzp-l-split-reversed & { | ||
| @include bidi(((float, right, left),)); | ||
| grid-template-areas: 'media body'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
|
|
||
| .mzp-l-split-reversed & { | ||
| @include bidi(((float, left, right),)); | ||
| .mzp-c-split-container & { // extra specificitly to over-ride the styles that make the float fall-back work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maintenance (non-blocking) is there still a float fallback here? we can remove float comments if fallback is fully removed
|
|
||
| @media #{$mq-md} { | ||
| .mzp-c-split { | ||
| // horizontal alignment works with float fallbacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maintenance (non-blocking) remove float fallback comments (if we've removed the float fallback)
| - `mzp-has-audio` | ||
|
|
||
| The default Card size with no other modifier classes is small: | ||
| - Small sized cards can contain images with 16:9, 3:2 or 1:1 aspect ratios. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs (non-blocking) Line above reads "The default Card size with no other modifier classes is small". Update default to medium.
| ### No Space: `mzp-t-section-heading-nospace` | ||
|
|
||
| Use this theme class to remove vertical padding from the section heading. This is | ||
| useful when you need tighter control over spacing in custom layouts or when the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 appreciate the "this is useful" detail
| maximum of 150 characters. | ||
| - name: Small | ||
| context: | ||
| class: 'mzp-has-aspect-1-1 mzp-has-audio' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs (non-blocking): otherwise there's no difference in the small/medium demo cards
| class: 'mzp-has-aspect-1-1 mzp-has-audio' | |
| class: 'mzp-c-card-small mzp-has-aspect-1-1 mzp-has-audio' |
Description
Refactor spacing system to use CSS custom properties. This is the third PR in a refactor of how we use CSS variables (#982), following the typography (#1107) and color (#1109) refactors. Introduces -
-theme-spacing-between-blockand--theme-spacing-between-inlinefor responsive layout spacing while retaining Sass$spacing-*variables for component-internal spacing. (We do need to standardize that but it's not nearly as straight forward)bidi-grid-spacing()mixingrid-gapfrom stylelint ignored shorthandsIssue
#982
Testing
Spot-check.
There are some expected visual regressions in section headings and the card documentation (but not the cards themselves). And the layout spacing has changed in some places where it needed standardization.
CHANGELOG.md.