Skip to content

Convert templates/ and nav/menu/footer family to logical properties - #1147

Open
stephaniehobson wants to merge 1 commit into
mainfrom
v23/logical-templates
Open

Convert templates/ and nav/menu/footer family to logical properties#1147
stephaniehobson wants to merge 1 commit into
mainfrom
v23/logical-templates

Conversation

@stephaniehobson

@stephaniehobson stephaniehobson commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Convert templates/ and nav/menu/footer family to logical properties

  • I have documented this change in the design system.
  • I have recorded this change in CHANGELOG.md.

Issue

Part of #1084

Testing

Spot check in RTL. I ran a visual regression locally and found no problems.

@stephaniehobson
stephaniehobson added this pull request to stack #1149 September 11, 2026 20:33
@stephaniehobson
stephaniehobson force-pushed the v23/logical-templates branch 2 times, most recently from f37ae80 to 27ba758 Compare October 2, 2026 02:05
stephaniehobson
stephaniehobson commented Oct 2, 2026
View reviewed changes
Comment thread assets/sass/protocol/components/_footer.scss
&:nth-child(odd) {
clear: inline-start;
@include bidi(((padding, 0 ($layout-md * 0.5) 0 0, 0 0 0 ($layout-md * 0.5)),));
padding-inline-end: $layout-md * 0.5;

@stephaniehobson stephaniehobson Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is going to drive me a little crazy that there is no logical property short hand for all 4 values.

Many changes in this PR no longer zero out padding that used to be explicitly removed by the previous declaration.

@knowler knowler Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ya, this ends up being a footgun for a lot of people. They keep using the padding shorthand either for zeroing or even the multi-value syntax, but then they mix the individual logical properties (and then never test). I end up just using padding-block and padding-inline or the longhands, unless I’m zeroing everything.

Base automatically changed from v23/logical-base to main October 5, 2026 17:44
@stephaniehobson
stephaniehobson marked this pull request as ready for review October 6, 2026 03:54
@stephaniehobson

stephaniehobson commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Visual regression tests run locally find no regressions so I'm opening this for review.

@stephaniehobson stephaniehobson added the Needs:Review 👋 Ready for Developer Review label Oct 6, 2026
@stephaniehobson
Convert templates/ and nav/menu/footer family to logical properties (#…
1d2d070 
…1084)

Second pass of the logical-properties migration -- the templates/ and
navigation/menu/footer component family, all built against current
main (post mixed-decls merge), not the older WS-4 branch state. 59 of
the 110 remaining @include bidi() calls removed:

  templates/_card-layout.scss    (15 -> 0)
  _navigation.scss               (11 -> 0)
  _footer.scss                   (10 -> 0)
  _menu-item.scss                (6 -> 0)
  _menu.scss                     (5 -> 0)
  _menu-list.scss                (4 -> 0)
  _sidebar-menu.scss             (4 -> 0)
  templates/_main-with-sidebar.scss (4 -> 0)

All eight files are now fully bidi()-free.

Notable non-mechanical cases:
- _navigation.scss / _menu-list.scss: several bidi() calls used the
  3-value "same property, different value per direction" form in
  pairs (e.g. two separate (padding-left, X, 0) / (padding-right, 0, X)
  tuples) rather than one 4-value tuple. Same underlying swap pattern,
  just spelled differently -- traced each pair through by hand to
  confirm which logical property they resolve to.
- _sidebar-menu.scss: one bidi() tuple paired a margin swap with a
  `transform: none / translateY(3px) rotate(180deg)` pair, flipping a
  ▸ triangle glyph to point the other way in RTL. transform has no
  logical/direction-aware equivalent (it's always in the element's own
  coordinate space), so that one stays an explicit [dir='rtl'] override
  -- only the margin half converted to margin-inline-start.
- _navigation.scss: two `background-position` bidi() calls stay
  physical with [dir='rtl'] overrides (same policy as the select rule
  in E1 -- no safe logical keyword syntax for background-position
  across this browser matrix). One of the two also had a bidi()
  tuple that was identical in both directions (dead weight, a pure
  no-op) -- collapsed to a single plain declaration.
- templates/_card-layout.scss, _menu.scss, _menu-item.scss: also
  converted several bare (non-bidi-wrapped) margin-left/margin-right:0
  pairs to margin-inline: 0 -- these were plain symmetric physical
  declarations sitting right next to the bidi() calls, in scope for
  the same inline-axis cleanup even though they weren't wrapped in the
  mixin.

Two real bugs found via a reported visual regression (menu-list
component rendering padding: 0 24px 0 4px locally vs. production's
padding: 0 24px 0 0) and fixed here, both the same class as the
label.mzp-u-inline bug found in the E1 commit -- a full 4-value
padding/margin shorthand implicitly zeroes every side it doesn't
otherwise set, and replacing it with a single logical longhand drops
that protection for the sides not touched:

- _menu-list.scss's `.is-details .mzp-c-menu-list-title button`: the
  original bidi() call's LTR value was the full shorthand
  "0 (16px + $spacing-sm) 0 0", explicitly zeroing padding-top/bottom/
  left. My conversion had only set padding-inline-end, so the button's
  padding-block and padding-inline-start fell through to the browser's
  UA default <button> padding (non-zero on every browser) instead of
  the intended 0. Fixed with padding-block: 0; padding-inline: 0 X.
- _footer.scss's `.mzp-c-footer-section:first-child`/`:last-child` at
  $mq-lg: the parent rule sets an explicit padding: 0 (X); shorthand
  (both inline sides non-zero), and :first-child/:last-child each
  need to zero out *one specific side* against that parent value --
  not restate the other side, which is what I'd written (a
  copy-paste-shaped mistake: I pattern-matched this diff's shape
  against a different, unrelated footer case that had no competing
  parent padding, and used the same-looking conversion without
  re-deriving it against this rule's actual cascade). Fixed to
  padding-inline-start: 0 / padding-inline-end: 0 respectively.

Given the severity, went back and systematically re-audited every
other bare "padding"/"margin"-shorthand-origin bidi() conversion
across both this commit and the E1 commit against the *actual*
parent-cascaded baseline (not just diff-shape pattern matching) --
checked ~20 selectors individually against true-original compiled
output. Everything else checked out: card-layout's, navigation's, and
menu-item's single-side overrides all rely on an explicit sibling
declaration (an earlier-cascading media query, or a base rule in the
same file) that already supplies the correct value for the side not
touched -- confirmed byte-for-byte via the same before/after compile
diff technique used throughout, this time checked per-selector rather
than by diff shape. Also confirmed the already-merged mixed-decls PR
doesn't have this bug class: it only did float/text-align keyword
swaps (no "sides" to drop), never converted a padding/margin
shorthand.

Branched directly off current main rather than the stale WS-4 chain
(D1/D2/E1 sit on pre-mixed-decls-merge main via the WS-3 tip) --
avoided converting bidi() calls against a structure that would need
re-reconciling again once that chain rebases. This PR has no file
overlap with D1/D2/E1's own scope.

Part of #1084.

Verified: npm run lint, npm test (47 specs, Firefox + Chrome),
npm run build-docs (523 items, no errors), npm run build-package, and
the exhaustive before/after compile diff described above all pass.
@stephaniehobson
stephaniehobson force-pushed the v23/logical-templates branch 2 times, most recently from fbc0f31 to 1d2d070 Compare October 7, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knowler knowler knowler left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Needs:Review 👋 Ready for Developer Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stephaniehobson @knowler