-
Notifications
You must be signed in to change notification settings - Fork 0
polishing #5
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
polishing #5
Conversation
WalkthroughBumped two devDependencies in the root Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/spacings/src/_variables.scss (3)
2-22: Namespace the base variable and add a safe fallback to avoid global collisions.
--spacingis very generic and lives in:root; easy to clash with app/global CSS. Recommend a namespaced base (keep--spacingas an alias for backward compatibility) and use a fallback in derived tokens so partial imports don’t break.Apply:
:root { - --spacing: 0.125rem; + /* Namespaced base + legacy alias */ + --ds-spacing: 0.125rem; + --spacing: var(--ds-spacing); - --spacing-00: 0px; /* 0rem */ - --spacing-01: var(--spacing); /* 0.125rem */ - --spacing-02: calc(var(--spacing) * 2); /* 0.25rem */ - --spacing-03: calc(var(--spacing) * 3); /* 0.375rem */ - --spacing-04: calc(var(--spacing) * 4); /* 0.5rem */ - --spacing-05: calc(var(--spacing) * 6); /* 0.75rem */ - --spacing-06: calc(var(--spacing) * 8); /* 1rem */ - --spacing-07: calc(var(--spacing) * 10); /* 1.25rem */ - --spacing-08: calc(var(--spacing) * 12); /* 1.5rem */ - --spacing-09: calc(var(--spacing) * 16); /* 2rem */ - --spacing-10: calc(var(--spacing) * 20); /* 2.5rem */ - --spacing-11: calc(var(--spacing) * 24); /* 3rem */ - --spacing-12: calc(var(--spacing) * 28); /* 3.5rem */ - --spacing-13: calc(var(--spacing) * 32); /* 4rem */ - --spacing-14: calc(var(--spacing) * 36); /* 4.5rem */ - --spacing-15: calc(var(--spacing) * 40); /* 5rem */ - --spacing-16: calc(var(--spacing) * 48); /* 6rem */ - --spacing-17: calc(var(--spacing) * 60); /* 7.5rem */ - --spacing-18: calc(var(--spacing) * 80); /* 10rem */ + --spacing-00: 0px; /* 0rem */ + --spacing-01: var(--ds-spacing, 0.125rem); /* 0.125rem */ + --spacing-02: calc(var(--ds-spacing, 0.125rem) * 2); /* 0.25rem */ + --spacing-03: calc(var(--ds-spacing, 0.125rem) * 3); /* 0.375rem */ + --spacing-04: calc(var(--ds-spacing, 0.125rem) * 4); /* 0.5rem */ + --spacing-05: calc(var(--ds-spacing, 0.125rem) * 6); /* 0.75rem */ + --spacing-06: calc(var(--ds-spacing, 0.125rem) * 8); /* 1rem */ + --spacing-07: calc(var(--ds-spacing, 0.125rem) * 10); /* 1.25rem */ + --spacing-08: calc(var(--ds-spacing, 0.125rem) * 12); /* 1.5rem */ + --spacing-09: calc(var(--ds-spacing, 0.125rem) * 16); /* 2rem */ + --spacing-10: calc(var(--ds-spacing, 0.125rem) * 20); /* 2.5rem */ + --spacing-11: calc(var(--ds-spacing, 0.125rem) * 24); /* 3rem */ + --spacing-12: calc(var(--ds-spacing, 0.125rem) * 28); /* 3.5rem */ + --spacing-13: calc(var(--ds-spacing, 0.125rem) * 32); /* 4rem */ + --spacing-14: calc(var(--ds-spacing, 0.125rem) * 36); /* 4.5rem */ + --spacing-15: calc(var(--ds-spacing, 0.125rem) * 40); /* 5rem */ + --spacing-16: calc(var(--ds-spacing, 0.125rem) * 48); /* 6rem */ + --spacing-17: calc(var(--ds-spacing, 0.125rem) * 60); /* 7.5rem */ + --spacing-18: calc(var(--ds-spacing, 0.125rem) * 80); /* 10rem */ }
4-4: Nit: Use unitless zero.Prefer
0over0px.- --spacing-00: 0px; /* 0rem */ + --spacing-00: 0; /* 0rem */
5-22: Optional: Generate tokens via SCSS to reduce drift.A tiny map + loop keeps scale definitions and comments in sync if the base changes.
$spacing-mults: ( ); :root { --ds-spacing: 0.125rem; --spacing: var(--ds-spacing); --spacing-00: 0; @each $k, $m in $spacing-mults { --spacing-#{$k}: calc(var(--ds-spacing, 0.125rem) * #{$m}); /* #{0.125rem * $m} */ } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/spacings/bun.lockis excluded by!**/*.lockpackages/spacings/dist/index.cssis excluded by!**/dist/**packages/spacings/dist/index.css.mapis excluded by!**/dist/**,!**/*.mappackages/spacings/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/spacings/package.json(1 hunks)packages/spacings/src/_variables.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/spacings/package.json
🔇 Additional comments (2)
packages/spacings/src/_variables.scss (2)
2-22: Solid tokenization; scale is consistent and math checks out.Deriving the whole scale from a single base is a good move; comments match the computed rem values.
2-2: No repo-wide collisions for--spacingfound.
Searched tracked files (git ls-files → ripgrep) for--spacingand the declaration pattern--spacing\s*:; no matches.
Summary by CodeRabbit