polishing #6
polishing #6
Conversation
WalkthroughMove font-stack CSS custom properties from Changes
Sequence Diagram(s)(Skipped — changes are stylesheet extraction and static build/config updates, not control-flow or runtime interactions.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/typography/src/_fonts.scss (1)
1-18: Optional: add a short usage note or example.A brief comment showing
font-family: var(--system-ui);helps consumers discover these tokens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/typography/bun.lockis excluded by!**/*.lockpackages/typography/dist/index.css.mapis excluded by!**/dist/**,!**/*.mappackages/typography/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/typography/package.json(2 hunks)packages/typography/src/_fonts.scss(1 hunks)packages/typography/src/index.scss(1 hunks)
🔇 Additional comments (1)
packages/typography/src/index.scss (1)
1-1: LGTM: importing fonts first is correct.This ensures CSS custom properties are available to later modules.
| { | ||
| "name": "@new-ui/typography", | ||
| "version": "0.1.8", | ||
| "version": "0.1.9", |
There was a problem hiding this comment.
🧩 Analysis chain
Version/devDeps bump looks fine; verify Node/Parcel compatibility and lockfile.
- Parcel 2.16 may require a newer Node runtime. Ensure CI/runtime Node version satisfies Parcel’s requirement.
- Make sure all @parcel/* plugins in the repo (if any) are on compatible 2.16.x to avoid subtle build issues.
- Refresh and commit your lockfile to prevent CI drift.
Run to audit engines, parcel plugins, and lockfile presence:
Also applies to: 42-46
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Node engines (if declared):"
fd -t f package.json | while read -r f; do
jq -r --arg f "$f" '(.engines.node // empty) as $n | if $n != null then "\($f): \($n)" else empty end' "$f"
done
echo -e "\nAll @parcel deps by version:"
rg -nP '"@parcel/[^"]+"\s*:\s*"[^"]+"' -g '!**/dist/**' -g '!**/node_modules/**'
echo -e "\nTop-level lockfiles:"
ls -1 pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || trueLength of output: 276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Collecting package.json files..."
mapfile -t files < <(fd -t f 'package.json' 2>/dev/null || find . -type f -name 'package.json' -not -path '*/node_modules/*' 2>/dev/null || true)
if [ ${#files[@]} -eq 0 ]; then
echo "No package.json files found"
else
echo "Found ${#files[@]} package.json files"
echo -e "\nNode engines (if declared):"
for f in "${files[@]}"; do
node_engine=$(jq -r '.engines.node // empty' "$f" 2>/dev/null || true)
if [ -n "$node_engine" ]; then
printf '%s: %s\n' "$f" "$node_engine"
fi
done
echo -e "\nAll @parcel deps by version (per package.json):"
for f in "${files[@]}"; do
echo "--- $f ---"
jq -r '((.dependencies // {}) + (.devDependencies // {}) + (.peerDependencies // {}))
| to_entries[]?
| select(.key|test("^@parcel/"))
| "\(.key): \(.value)"' "$f" 2>/dev/null || true
done
echo -e "\npackages/typography/package.json version (if present):"
if [ -f packages/typography/package.json ]; then
jq -r '.version // empty' packages/typography/package.json || true
else
echo "not found"
fi
fi
echo -e "\nTop-level lockfiles (root):"
for lock in pnpm-lock.yaml package-lock.json yarn.lock; do
[ -f "$lock" ] && echo "$lock"
doneLength of output: 89
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Safe search for package.json files (no process substitution)..."
tmpfile=$(mktemp)
cleanup() { rm -f "$tmpfile"; }
trap cleanup EXIT
if command -v fd >/dev/null 2>&1; then
fd -t f 'package.json' 2>/dev/null > "$tmpfile" || true
else
find . -type f -name 'package.json' -not -path '*/node_modules/*' 2>/dev/null > "$tmpfile" || true
fi
if [ ! -s "$tmpfile" ]; then
echo "No package.json files found"
exit 0
fi
mapfile -t files < "$tmpfile"
echo "Found ${#files[@]} package.json files"
if ! command -v jq >/dev/null 2>&1; then
echo "jq is not installed; cannot parse JSON fields."
fi
echo -e "\nNode engines (if declared):"
for f in "${files[@]}"; do
jq -r '.engines.node // empty' "$f" 2>/dev/null || true
if [ "${PIPESTATUS[0]:-0}" -ne 0 ]; then
echo "(failed to read $f)"
else
node_engine=$(jq -r '.engines.node // empty' "$f" 2>/dev/null || true)
[ -n "$node_engine" ] && printf '%s: %s\n' "$f" "$node_engine"
fi
done
echo -e "\nAll @parcel deps by version (per package.json):"
for f in "${files[@]}"; do
echo "--- $f ---"
jq -r '((.dependencies // {}) + (.devDependencies // {}) + (.peerDependencies // {}))
| to_entries[]?
| select(.key|test("^@parcel/"))
| "\(.key): \(.value)"' "$f" 2>/dev/null || true
done
echo -e "\npackages/typography/package.json version (if present):"
if [ -f packages/typography/package.json ]; then
jq -r '.version // empty' packages/typography/package.json || true
else
echo "not found"
fi
echo -e "\nTop-level lockfiles (root):"
for lock in pnpm-lock.yaml package-lock.json yarn.lock; do
[ -f "$lock" ] && echo "$lock"
doneLength of output: 777
Action required — align @parcel/ to 2.16.x, declare Node engine, and add/commit lockfile*
- No engines.node declared in any package.json — add engines.node in the repo root (or enforce the Node version in CI) so runtime satisfies Parcel 2.16.x.
- Mismatched @parcel/* versions found — update so all plugins use the same 2.16.x series:
- package.json (root): @parcel/transformer-sass: ^2.16.0
- packages/typography/package.json: @parcel/transformer-sass: ^2.16.0
- packages/colors/package.json: @parcel/transformer-sass: ^2.14.4
- packages/effects/package.json: @parcel/transformer-sass: ^2.14.4
- packages/grid/package.json: @parcel/transformer-sass: ^2.14.4
- packages/reset/package.json: @parcel/transformer-sass: ^2.14.4
- packages/spacings/package.json: @parcel/transformer-css: ^2.14.4, @parcel/transformer-sass: ^2.14.4
- No top-level lockfile detected — run your package-manager install (npm/pnpm/yarn), refresh the lockfile, and commit it to prevent CI drift.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/reset/bun.lockis excluded by!**/*.lockpackages/reset/dist/index.cssis excluded by!**/dist/**packages/reset/dist/index.css.mapis excluded by!**/dist/**,!**/*.mappackages/reset/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/reset/package.json(2 hunks)packages/typography/src/_variables.scss(0 hunks)
💤 Files with no reviewable changes (1)
- packages/typography/src/_variables.scss
🔇 Additional comments (2)
packages/reset/package.json (2)
3-3: Semver bump looks fine; remember release hygiene.
- Minor version to 0.1.0 aligns with tooling updates.
- Please update CHANGELOG and tag the release when merging.
38-38: Align Parcel version across workspaces and refresh lockfile.
- Make sure all packages use
parcel@^2.16.0to avoid duplicated toolchains.- After merge, run a clean install to regenerate the lockfile with the new tool versions.
| "homepage": "https://github.com/NewDesignFile/foundations#readme", | ||
| "devDependencies": { | ||
| "@parcel/transformer-sass": "^2.14.4", | ||
| "@parcel/transformer-sass": "^2.16.0", |
There was a problem hiding this comment.
🧩 Analysis chain
Check sass peer and Node engine for Parcel/Sass pipeline.
@parcel/transformer-sass@^2.16.0typically expects asasspeer. If you’re relying on workspace hoisting, consider adding an explicit devDependency here to avoid fragility.- Ensure CI uses a Node version compatible with Parcel 2.16 (generally Node 18+).
Run this script to verify across the monorepo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Packages with transformer-sass but missing sass =="
fd -a package.json packages | while read -r f; do
trans=$(jq -r '.devDependencies["@parcel/transformer-sass"] // empty' "$f")
if [[ -n "$trans" ]]; then
sass=$(jq -r '.devDependencies.sass // empty' "$f")
if [[ -z "$sass" ]]; then
echo "Missing sass -> $f (transformer-sass: $trans)"
fi
fi
done
echo
echo "== Parcel versions by package =="
fd -a package.json packages | while read -r f; do
parcel=$(jq -r '.devDependencies.parcel // empty' "$f")
if [[ -n "$parcel" ]]; then
echo "$f : parcel $parcel"
fi
done
echo
echo "== Root engines.node (if any) =="
jq -r '.engines.node // "NO engines.node in root package.json"' package.json 2>/dev/null || true
echo
echo "== .nvmrc / .node-version (if present) =="
[ -f .nvmrc ] && { echo -n ".nvmrc: "; cat .nvmrc; } || echo "No .nvmrc"
[ -f .node-version ] && { echo -n ".node-version: "; cat .node-version; } || echo "No .node-version"Length of output: 1178
Add explicit sass devDependency to packages using @parcel/transformer-sass and declare Node engine (Node 18+)
- Add devDependency
sassto: packages/colors/package.json, packages/effects/package.json, packages/grid/package.json, packages/reset/package.json, packages/spacings/package.json, packages/typography/package.json (each lists@parcel/transformer-sassbut lackssass). - Declare
engines.nodein root package.json (or add .nvmrc/.node-version) and ensure CI uses Node 18+; parcel versions present: packages/reset & packages/typography → ^2.16.0, others → ^2.14.4 — verify Node 18+ compatibility across these.
🤖 Prompt for AI Agents
In packages/reset/package.json around line 36, the package lists
@parcel/transformer-sass but does not declare the required peer/devDependency
"sass"; add "sass" as a devDependency (use a current stable version compatible
with your Parcel v2.x) to packages/reset/package.json, and repeat for the other
listed packages (colors, effects, grid, spacings, typography); additionally
declare engines.node (Node 18+) in the repository root package.json or add an
.nvmrc/.node-version file and ensure CI is configured to use Node 18+; finally
verify Parcel v2.14/2.16 compatibility with Node 18 across those packages.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/grid/bun.lockis excluded by!**/*.lockpackages/grid/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/grid/package.json(1 hunks)
🔇 Additional comments (2)
packages/grid/package.json (2)
40-40: No extrasassdevDependency required — @parcel/transformer-sass already depends onsass.
@parcel/transformer-sass declaressass(dart-sass) in its dependencies for Parcel 2.16, so addingsassto packages/grid/package.json is unnecessary.Likely an incorrect or invalid review comment.
42-42: Parcel 2.16 upgrade — ensure Node engine compatibility and repo-wide Parcel/transformer sync.
- Parcel v2.16 requires Node >=16.0.0 — add an engines.node >=16.0.0 (repo root or per-package) and ensure CI uses the same Node image.
- Repo shows mixed Parcel/transformer versions; align to a single version (recommend ^2.16.0):
- ^2.16.0: packages/typography/package.json, packages/grid/package.json, packages/reset/package.json (bun.lock entries present).
- ^2.14.4: packages/spacings/package.json, packages/effects/package.json, packages/colors/package.json (bun.lock entries present).
- @parcel/transformer-sass is also mixed (2.16.0 vs 2.14.4) — sync this alongside parcel.
- Re-run CI and a repo-wide audit after syncing. Quick audit commands used here:
- rg -nP '"parcel"\s*:\s*"^?2..+"' -C1 packages
- rg -nP '"@parcel/transformer-sass"\s*:\s*"^?2..+"' -C1 packages
packages/grid/package.json
Outdated
| "devDependencies": { | ||
| "@new-ui/reset": "^0.0.8", | ||
| "@parcel/transformer-sass": "^2.14.4", | ||
| "@new-ui/reset": "^0.0.9", |
There was a problem hiding this comment.
🧩 Analysis chain
Reset bump looks fine; verify workspace alignment and refresh lockfile.
Ensure all packages reference the same @new-ui/reset range and re-generate the lockfile to avoid version skew.
Run to audit versions across the monorepo:
🏁 Script executed:
#!/bin/bash
# Where: repo root
rg -nP '"@new-ui/reset"\s*:\s*".*"' -C1 packages
jq -r '[.name, .version] | @tsv' packages/reset/package.json 2>/dev/null || trueLength of output: 393
Align @new-ui/reset version & refresh lockfile.
packages/grid/package.json (devDependencies ~line 39) declares "@new-ui/reset": "^0.0.9" while packages/reset/package.json is version 0.1.0; packages/grid/bun.lock still references ^0.0.9. Update packages/grid/package.json to "^0.1.0" (or use "workspace:*"), reinstall to regenerate lockfiles, and confirm no other packages reference ^0.0.9.
🤖 Prompt for AI Agents
In packages/grid/package.json around line 39, devDependency "@new-ui/reset" is
pinned to "^0.0.9" which is out of sync with packages/reset/package.json (0.1.0)
and bun.lock; update the dependency to "^0.1.0" or use "workspace:*" in
packages/grid/package.json, then reinstall dependencies to regenerate lockfiles
(bun.lock / package-lock / pnpm-lock as applicable) and run a repo-wide search
to confirm no other packages or lockfiles still reference "^0.0.9".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/effects/README.md (1)
14-15: Prefer pinning a version over @latest for production CDN usage.@latest can introduce breaking changes without notice. Recommend pinning a semver (e.g., @0.x.y) and, if pinning, consider adding SRI. Keep @latest only for quickstart/demos and note the trade‑off in the README.
Here’s a safer snippet:
-<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@new-ui/effects@latest/dist/index.css"> +<!-- For production, pin a version --> +<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@new-ui/effects@0.1.0/dist/index.css"> +<!-- Optionally add SRI when version is pinned --> +<!-- <link rel="stylesheet" href="...@0.1.0/dist/index.css" integrity="sha256-..." crossorigin="anonymous"> -->packages/spacings/README.md (1)
14-16: Same note: avoid @latest in production docs.Mirror the effects README guidance: pin a version in the example and optionally add SRI; keep @latest only for sandbox/demos with a cautionary note.
-<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@new-ui/spacings@latest/dist/index.css"> +<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@new-ui/spacings@0.1.0/dist/index.css">packages/grid/src/_grid.scss (5)
2-2: Remove unused Sass module import."sass:math" isn’t referenced; drop it to reduce compile overhead.
-@use "sass:math";
96-105: Make fixed-width columns non-growing to avoid unexpected expansion.With %col-base setting flex: 1 0 0%, width classes can still grow; prefer locking width via flex: 0 0 .
- &--#{$i} { - flex-basis: calc(100% * #{$i} / 12); - max-width: calc(100% * #{$i} / 12); - } + &--#{$i} { + flex: 0 0 calc(100% * #{$i} / 12); + max-width: calc(100% * #{$i} / 12); + }
71-77: Clarify intent of “.col--” baseline rules (100% at breakpoint).These set columns to 100% width at each token. Is the intent “stack at and above token unless an explicit width is provided”? If so, consider documenting or generating “.col--md-6” style utilities for per‑breakpoint sizing to complete the API.
125-127: Avoid will-change unless profiling proves benefit.will-change: margin can keep layers alive and increase memory/painting cost. Recommend removing or gating to interactive states.
-.col--push-left, -.col--push-right { - will-change: margin; - contain: layout; -} +.col--push-left, +.col--push-right { + contain: layout; +}
131-135: prefers-reduced-motion shouldn’t alter layout direction.Reduced‑motion is for animation/transition effects; flipping .grid--reverse to row changes content order and may surprise users. Recommend removing or restricting to animated reversals only.
-@media (prefers-reduced-motion: reduce) { - .grid--reverse { - flex-direction: row; - } -} +/* Consider handling motion via transitions/animations only, not layout direction */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
packages/colors/bun.lockis excluded by!**/*.lockpackages/colors/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackages/effects/bun.lockis excluded by!**/*.lockpackages/effects/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackages/grid/bun.lockis excluded by!**/*.lockpackages/grid/dist/index.cssis excluded by!**/dist/**packages/grid/dist/index.css.mapis excluded by!**/dist/**,!**/*.mappackages/typography/dist/index.cssis excluded by!**/dist/**packages/typography/dist/index.css.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (9)
packages/colors/package.json(2 hunks)packages/effects/README.md(1 hunks)packages/effects/package.json(1 hunks)packages/grid/README.md(1 hunks)packages/grid/package.json(2 hunks)packages/grid/src/_grid.scss(1 hunks)packages/spacings/README.md(1 hunks)packages/typography/README.md(1 hunks)packages/typography/src/_fonts.scss(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/typography/README.md
- packages/grid/README.md
- packages/effects/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/typography/src/_fonts.scss
- packages/grid/package.json
🔇 Additional comments (4)
packages/grid/src/_grid.scss (3)
47-58: Gap + fallback approach looks solid.grid-gutter-reset with gap and the @supports fallback correctly reconcile padding/margins and avoid double gutters.
Also applies to: 136-142
16-23: Gutter reset is correct and safely scoped.
111-122: Visibility utilities look good; add an a11y note.Rules are correct with non‑overlapping ranges. Ensure consumers pair with aria-hidden when hiding meaningful content.
packages/colors/package.json (1)
39-43: Align Parcel versions, confirm Node engine, and refresh Browserslist DB.
- Parcel/@parcel/transformer-sass mismatch — packages/spacings and packages/effects use ^2.14.4; colors, grid, typography, reset use ^2.16.0. Pick one version and align across the monorepo.
- caniuse-lite is only declared in packages/colors/package.json (^1.0.30001743). After bumping, run locally and commit lockfile: npx update-browserslist-db@latest.
- Script run produced jq errors when reading package.json "engines"; ensure package.json/CI declare Node >=16 (recommended for Parcel’s watcher) or confirm CI uses Node ≥16.
"scripts": { + "postinstall": "npx update-browserslist-db@latest || true", "watch": "parcel watch", "build": "parcel build" }
| "version": "2.0.2", | ||
| "description": "New UI colors", |
There was a problem hiding this comment.
🧩 Analysis chain
Patch bump + microcopy tweak look good. Please tag and verify references.
Semver patch to 2.0.2 and description punctuation are fine. Make sure any hard‑coded CDN/version references (if any remain) aren’t still pointing at 2.0.1.
Run to spot stray hard-coded versions and confirm a changelog/tag exist:
🏁 Script executed:
#!/bin/bash
# Find hard-coded package versions/usages
rg -nP '@new-ui/colors@|@new-ui/colors[^@]?\b' -S -C1
# Look for a changelog and recent tag for colors
fd -i --full-path '(CHANGELOG.md|changelog.md|news.md)'
git tag --list | rg -n '^@new-ui/colors@?2\.0\.2$' || echo "Tag @new-ui/colors@2.0.2 missing"Length of output: 1416
Patch bump + microcopy OK — update remaining refs & add release tag
- Root package.json still depends on "@new-ui/colors": "^2.0.1" (package.json:49) — update to ^2.0.2 and regenerate/commit lockfile(s).
- Root bun.lock contains @new-ui/colors@2.0.1 — regenerate and commit updated lockfile(s).
- No changelog found and git tag for 2.0.2 is missing — add a changelog entry and create the release tag (use repo tag convention).
🤖 Prompt for AI Agents
In packages/colors/package.json around lines 3 to 4, the package version was
bumped to 2.0.2 but other repo artifacts still reference 2.0.1; update the root
package.json dependency for "@new-ui/colors" (line ~49) to "^2.0.2", regenerate
and commit updated lockfile(s) (e.g., bun.lock / package-lock.json /
pnpm-lock.yaml as applicable), add a short changelog entry documenting the 2.0.2
release in the repo's changelog location, and create the git release tag
following the repository's tag convention (e.g., v2.0.2) before committing and
pushing the changes.
| @each $breakpoint, $value in $breakpoints { | ||
| @include breakpoint($breakpoint) { | ||
| max-width: calc(#{$value} - #{$base}); | ||
| } | ||
| } |
There was a problem hiding this comment.
Container width off by one side of padding (should subtract 2× base).
max-width currently subtracts only $base, but the container adds left and right padding ($base each). Subtract 2×$base to keep the container within the intended breakpoint width.
- max-width: calc(#{$value} - #{$base});
+ max-width: calc(#{$value} - #{2 * $base});📝 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.
| @each $breakpoint, $value in $breakpoints { | |
| @include breakpoint($breakpoint) { | |
| max-width: calc(#{$value} - #{$base}); | |
| } | |
| } | |
| @each $breakpoint, $value in $breakpoints { | |
| @include breakpoint($breakpoint) { | |
| max-width: calc(#{$value} - #{2 * $base}); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/grid/src/_grid.scss around lines 41 to 45, the media-query container
max-width only subtracts $base once but the container applies left and right
padding of $base each; update the calc to subtract 2×$base (e.g., calc(#{$value}
- #{2 * $base})) so the max-width accounts for both side paddings and keeps the
container inside the breakpoint width.
Summary by CodeRabbit
New Features
Chores