refactor(css): logical property audit — replace physical with logical equivalents - #321
Merged
Merged
Conversation
… equivalents Replace all remaining physical CSS properties with their logical equivalents across core/ for correct RTL and vertical writing-mode behaviour: - scroll-margin-top → scroll-margin-block-start (headings, :target) - scroll-padding-top → scroll-padding-block-start (html) - overflow-x → overflow-inline (pre, .sf-prose table, .sf-switcher--no-wrap, .sf-reel) - overflow-y → overflow-block (.sf-scroll-shadow, .sf-scroll-snap) - overscroll-behavior-x/y → overscroll-behavior-inline/block (.sf-reel, .sf-scroll-snap) - scroll-snap-type: x/y → inline/block (.sf-reel, .sf-scroll-snap) - width/max-width → inline-size/max-inline-size (.sf-prose table + .sf-not-prose revert) optional/legacy.css overflow-y: scroll is intentionally kept — it is a @supports fallback for browsers that lack scrollbar-gutter: stable, which also lack overflow-block; converting it would break the fallback. Closes the "logical property audit" pre-v1.0 roadmap item. https://claude.ai/code/session_01RL75QMbe2zPUb75ndCf8jh
Adds tests/token-semantic.spec.js — Playwright assertions for 21 key
semantic tokens across both light and dark mode:
• 10 color tokens resolved via canvas to sRGB {r,g,b} integers, tested
separately for light and dark (catches palette shifts and contrast changes)
• 11 scale/geometry tokens stored as raw CSS property strings (catches fluid
engine formula changes, radius, border-width, and font-weight regressions)
Snapshot file tests/token-semantic.snapshot.json is generated on first run
and committed here with the v0.5.41 baseline. Any future value change
requires an explicit snapshot update + CHANGELOG entry.
Also marks both pre-v1.0 roadmap items complete (logical property audit was
done in the previous commit; snapshot tests done here) and bumps the roadmap
current version to 0.5.42.
https://claude.ai/code/session_01RL75QMbe2zPUb75ndCf8jh
docs/api-index.{json,md} regenerated — picks up the overflow-block
description update in .sf-scroll-shadow from the logical property audit.
package-lock.json updated after npm install (Playwright browser install).
https://claude.ai/code/session_01RL75QMbe2zPUb75ndCf8jh
|
Warning Review limit reached
More reviews will be available in 50 minutes and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request converts the SLASHED design system from physical CSS properties to logical equivalents, enabling better support for writing-mode variations (RTL/LTR). Core stylesheets ( ChangesLogical Properties Migration and Token Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| const toRGB = (cssColor) => { | ||
| ctx.clearRect(0, 0, 1, 1); | ||
| ctx.fillStyle = '#000'; // reset blending baseline |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/base.css (1)
132-133: Updateoverflow-inlinebrowser support expectations (Safari)
overflow-inline(the CSS property used here) isn’t supported in Safari 15.x/16.x—Safari support starts at 26.0 (Chrome 135+, Firefox 69+). If you must support older Safari, add a fallback (e.g.,overflow-x) so inline-axis overflow doesn’t misbehave silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/base.css` around lines 132 - 133, The CSS uses the overflow-inline property which isn’t supported in older Safari; add a fallback using the corresponding physical axis property (e.g., add overflow-x: auto;) before the overflow-inline declaration so browsers that don’t support overflow-inline (Safari 15/16) still get correct inline-axis overflow behavior; keep the existing overflow-inline: auto; after the fallback to preserve modern behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/macros.css`:
- Around line 178-180: The test currently asserts expect(cs).toContain('y') for
the .sf-scroll-snap rule but the CSS uses scroll-snap-type: block mandatory
which computes to 'block mandatory'; update the test assertion to look for
'block' (e.g. expect(cs).toContain('block')) or otherwise check for the computed
value 'block mandatory' instead of 'y' so it matches the actual computed
scroll-snap-type produced by the .sf-scroll-snap rule; alternatively, if you
prefer to keep the test, change the CSS value for scroll-snap-type to an
axis-based token that computes to 'y', but the simpler fix is to update the test
expectation.
In `@docs/api-index.md`:
- Line 1055: The docs entry for the `.sf-scroll-shadow` macro uses physical-axis
phrasing; update the description to use block-axis/logical-property wording:
replace "Scroll shadow | Top + bottom mask gradient that reveals when content is
scrolled inside a vertical-scroll container. Pure CSS, no JS. Combine with
overflow-block: auto on the same element." with a block-axis phrasing such as
"Scroll shadow | Mask gradient at the block-start and block-end that reveals
when content is scrolled along the block axis. Pure CSS, no JS. Combine with
overflow-block: auto on the same element. Override:
style=\"--sf-scroll-shadow-size: 3rem\"" so the entry references
block-start/block-end and scrolling along the block axis and keeps the existing
override note; ensure the symbol `.sf-scroll-shadow` remains unchanged.
In `@tests/token-semantic.spec.js`:
- Around line 117-121: The test builds the current mapping with only light.raw
so it never asserts raw-token invariance; update the test in
tests/token-semantic.spec.js to include dark.raw in the current object (e.g.,
set both 'raw.light' and 'raw.dark' or keep a 'raw' entry for both modes) and
add/adjust the assertion(s) that compare raw tokens (the invariance check around
the current object and existing expectations) to explicitly verify that
light.raw and dark.raw are identical (or that raw is mode-insensitive), ensuring
the suite fails if dark.raw diverges from light.raw; modify the constructor of
the current object (the literal using 'colors.light', 'colors.dark' and 'raw')
and the corresponding assertions that reference raw to perform this equality
check.
---
Nitpick comments:
In `@core/base.css`:
- Around line 132-133: The CSS uses the overflow-inline property which isn’t
supported in older Safari; add a fallback using the corresponding physical axis
property (e.g., add overflow-x: auto;) before the overflow-inline declaration so
browsers that don’t support overflow-inline (Safari 15/16) still get correct
inline-axis overflow behavior; keep the existing overflow-inline: auto; after
the fallback to preserve modern behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a9f66c7-1fec-4977-aa9f-2d9edadd603f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
core/base.csscore/layout.csscore/macros.csscore/reset.cssdocs/api-index.jsondocs/api-index.mddocs/roadmap.mdtests/token-semantic.snapshot.jsontests/token-semantic.spec.js
Comment on lines
+178
to
+180
| overflow-block: auto; | ||
| scroll-snap-type: block mandatory; | ||
| overscroll-behavior-block: contain; |
There was a problem hiding this comment.
Test expects scrollSnapType to contain 'y', but scroll-snap-type: block computes differently.
The test at tests/macros.spec.js:154-169 asserts expect(cs).toContain('y') for .sf-scroll-snap. However, scroll-snap-type: block mandatory computes to 'block mandatory' in getComputedStyle(), not 'y mandatory'. The test will fail unless updated to check for 'block' instead of 'y'.
#!/bin/bash
# Verify the test assertion that will break
rg -n -B2 -A8 "scrollSnapType" tests/macros.spec.js🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/macros.css` around lines 178 - 180, The test currently asserts
expect(cs).toContain('y') for the .sf-scroll-snap rule but the CSS uses
scroll-snap-type: block mandatory which computes to 'block mandatory'; update
the test assertion to look for 'block' (e.g. expect(cs).toContain('block')) or
otherwise check for the computed value 'block mandatory' instead of 'y' so it
matches the actual computed scroll-snap-type produced by the .sf-scroll-snap
rule; alternatively, if you prefer to keep the test, change the CSS value for
scroll-snap-type to an axis-based token that computes to 'y', but the simpler
fix is to update the test expectation.
| | `.sf-scrim--full` | PUBLIC | macro | Scrim | text anchored at the bottom (default) | | ||
| | `.sf-scrim--top` | PUBLIC | macro | Scrim | Lift content above the scrim, but leave media below it so the gradient actually darkens the picture. | | ||
| | `.sf-scroll-shadow` | PUBLIC | macro | Scroll shadow | Top + bottom mask gradient that reveals when content is scrolled inside a vertical-scroll container. Pure CSS, no JS. Combine with overflow-y: auto on the same element. Override: style="--sf-scroll-shadow-size: 3rem" | | ||
| | `.sf-scroll-shadow` | PUBLIC | macro | Scroll shadow | Top + bottom mask gradient that reveals when content is scrolled inside a vertical-scroll container. Pure CSS, no JS. Combine with overflow-block: auto on the same element. Override: style="--sf-scroll-shadow-size: 3rem" | |
There was a problem hiding this comment.
Use block-axis wording here.
The description still uses physical-axis phrasing (“vertical-scroll”, “top + bottom”), which is misleading in a logical-property audit. Please reword it in block-axis terms so the docs stay correct for vertical writing modes.
♻️ Suggested wording
- Top + bottom mask gradient that reveals when content is scrolled inside a vertical-scroll container.
+ Block-axis mask gradient that reveals when content is scrolled inside a block-scroll container.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `.sf-scroll-shadow` | PUBLIC | macro | Scroll shadow | Top + bottom mask gradient that reveals when content is scrolled inside a vertical-scroll container. Pure CSS, no JS. Combine with overflow-block: auto on the same element. Override: style="--sf-scroll-shadow-size: 3rem" | | |
| | `.sf-scroll-shadow` | PUBLIC | macro | Scroll shadow | Block-axis mask gradient that reveals when content is scrolled inside a block-scroll container. Pure CSS, no JS. Combine with overflow-block: auto on the same element. Override: style="--sf-scroll-shadow-size: 3rem" | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/api-index.md` at line 1055, The docs entry for the `.sf-scroll-shadow`
macro uses physical-axis phrasing; update the description to use
block-axis/logical-property wording: replace "Scroll shadow | Top + bottom mask
gradient that reveals when content is scrolled inside a vertical-scroll
container. Pure CSS, no JS. Combine with overflow-block: auto on the same
element." with a block-axis phrasing such as "Scroll shadow | Mask gradient at
the block-start and block-end that reveals when content is scrolled along the
block axis. Pure CSS, no JS. Combine with overflow-block: auto on the same
element. Override: style=\"--sf-scroll-shadow-size: 3rem\"" so the entry
references block-start/block-end and scrolling along the block axis and keeps
the existing override note; ensure the symbol `.sf-scroll-shadow` remains
unchanged.
…ion sync
After the logical property audit, three test assertions needed updating:
- tests/layout.spec.js, tests/demo-visual.spec.js:
scroll-snap-type on .sf-reel changed from 'x mandatory' to 'inline mandatory';
update toContain('x') → toContain('inline') in both reel snap-type assertions.
- tests/token-semantic.spec.js:
Canvas OKLCH→sRGB conversion differs by ±1 across browser engines — restrict
the two color snapshot assertions to Chromium (where the baseline was captured)
via test.skip on Firefox/WebKit. Also add explicit dark.raw === light.raw
invariance check to verify raw tokens are truly mode-insensitive.
- docs/roadmap.md:
Revert version string to 0.5.41 to pass check-version-sync (package.json
is the source of truth; roadmap version is updated by npm run version-sync).
https://claude.ai/code/session_01RL75QMbe2zPUb75ndCf8jh
This was referenced Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace all remaining physical CSS properties with their logical
equivalents across core/ for correct RTL and vertical writing-mode
behaviour:
optional/legacy.css overflow-y: scroll is intentionally kept — it is a
@supports fallback for browsers that lack scrollbar-gutter: stable, which
also lack overflow-block; converting it would break the fallback.
Closes the "logical property audit" pre-v1.0 roadmap item.
https://claude.ai/code/session_01RL75QMbe2zPUb75ndCf8jh
Summary by CodeRabbit
Refactor
Tests
Documentation
Chores