fix: paper theme skeleton loader and navbar improvements#52
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Paper theme receives a navbar skeleton loader. A Radix UI icon dependency is added, CSS layout rules are updated for full-width containers and loader styling, Page component icons are swapped to use Radix Cross2Icon, and the PageSkeleton component now displays a navbar region with two skeleton loader placeholders. ChangesNavbar Skeleton Loader with Icon Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/chronicle/src/themes/paper/Skeleton.tsx (1)
3-3: 💤 Low valueOptional: prefer
clsx(or a template literal) overcxfromclass-variance-authorityfor simple class merging.
cxfromclass-variance-authorityworks correctly here, butclass-variance-authorityis primarily designed for variant-driven class generation viacva(). Pulling incxjust to join two CSS module classes conflates a utility concern with a design-system concern. Aclsximport (already transitively available) or a plain template string keeps the intent clearer and avoids a potentially confusing dependency.♻️ Proposed refactor
-import { cx } from 'class-variance-authority';Then inline the class composition:
- <div className={cx(styles.navLeft, styles.navbarLoaderWrapper)}> + <div className={`${styles.navLeft} ${styles.navbarLoaderWrapper}`}> ... - <div className={cx(styles.navRight, styles.navbarLoaderWrapper)}> + <div className={`${styles.navRight} ${styles.navbarLoaderWrapper}`}>Also applies to: 9-14
🤖 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 `@packages/chronicle/src/themes/paper/Skeleton.tsx` at line 3, The file imports cx from class-variance-authority just to merge simple CSS module classes; remove that import and either import clsx from 'clsx' or use a template literal to combine classes in the Skeleton component; update every use of cx (around the className composition lines referenced) to use clsx(...) or `${styles.foo} ${styles.bar}` and ensure the import statement is replaced/removed accordingly so only the needed utility is referenced.
🤖 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.
Nitpick comments:
In `@packages/chronicle/src/themes/paper/Skeleton.tsx`:
- Line 3: The file imports cx from class-variance-authority just to merge simple
CSS module classes; remove that import and either import clsx from 'clsx' or use
a template literal to combine classes in the Skeleton component; update every
use of cx (around the className composition lines referenced) to use clsx(...)
or `${styles.foo} ${styles.bar}` and ensure the import statement is
replaced/removed accordingly so only the needed utility is referenced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd94eac9-983f-46e4-ad0e-f936e31f6755
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
packages/chronicle/package.jsonpackages/chronicle/src/themes/paper/Page.module.csspackages/chronicle/src/themes/paper/Page.tsxpackages/chronicle/src/themes/paper/Skeleton.tsx
Summary
--rs-space-7and set width to 100%@heroiconswith@radix-ui/react-iconsfor settings/close icons (MixerHorizontalIcon, Cross2Icon) for consistencyTest plan
bun run dev:examples:basicand verify skeleton loader shows navbar placeholder🤖 Generated with Claude Code