Skip to content

fix: paper theme skeleton loader and navbar improvements#52

Merged
rsbh merged 3 commits intomainfrom
fix_paper_design_skeleton
May 8, 2026
Merged

fix: paper theme skeleton loader and navbar improvements#52
rsbh merged 3 commits intomainfrom
fix_paper_design_skeleton

Conversation

@rsbh
Copy link
Copy Markdown
Member

@rsbh rsbh commented May 8, 2026

Summary

  • Add navbar skeleton to paper theme page loader for smoother loading experience
  • Fix paper content card margins — add horizontal spacing with --rs-space-7 and set width to 100%
  • Replace @heroicons with @radix-ui/react-icons for settings/close icons (MixerHorizontalIcon, Cross2Icon) for consistency

Test plan

  • Run bun run dev:examples:basic and verify skeleton loader shows navbar placeholder
  • Verify paper theme content card has proper horizontal margins
  • Check settings panel icons render correctly (open/close)
  • Verify dark mode works for all changes

🤖 Generated with Claude Code

@vercel
Copy link
Copy Markdown

vercel Bot commented May 8, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
chronicle Ready Ready Preview, Comment May 8, 2026 4:21am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Style

    • Refined page container layout to use full width with optimized content spacing and margins
    • Updated interface icons for improved visual consistency
  • New Features

    • Added navigation skeleton loaders to provide visual feedback during page content loading

Walkthrough

The 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.

Changes

Navbar Skeleton Loader with Icon Updates

Layer / File(s) Summary
Dependencies
packages/chronicle/package.json
Adds @radix-ui/react-icons (^1.3.2) to dependencies.
CSS Layout and Loader Styling
packages/chronicle/src/themes/paper/Page.module.css
.main width changed to 100%; .content now uses viewport-based min-height and 4-side margin shorthand; .loader adds flex: 1 and adjusted margins; .navbarLoaderWrapper set to width: 30%.
Icon Imports and Usage
packages/chronicle/src/themes/paper/Page.tsx
Imports Heroicons (EyeIcon, SunIcon, MoonIcon, ArrowLeftIcon, ArrowRightIcon) and Cross2Icon from Radix icons; settings close button uses Cross2Icon when open.
Navbar Skeleton Component
packages/chronicle/src/themes/paper/Skeleton.tsx
Imports cx from class-variance-authority and adds a navbar region containing two side-by-side Skeleton components with navbarLoaderWrapper and loader CSS classes; existing skeleton sections remain unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • raystack/chronicle#44: Modifies the same Paper theme Page, Page.module.css, and Skeleton files with icon and style updates.
  • raystack/chronicle#48: Modifies the Page component and its icon imports and rendering in the Paper theme.
  • raystack/chronicle#50: Updates the paper theme's Skeleton.tsx and Page.module.css with skeleton and CSS changes.

Suggested reviewers

  • rohilsurana
  • rohanchkrabrty
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main changes: paper theme skeleton loader improvements and navbar enhancements, matching the changeset content.
Description check ✅ Passed The description provides specific details about the changes including navbar skeleton, content margins, icon library replacements, and test plan, directly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix_paper_design_skeleton

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rsbh rsbh requested review from rohanchkrabrty and rohilsurana May 8, 2026 04:22
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/chronicle/src/themes/paper/Skeleton.tsx (1)

3-3: 💤 Low value

Optional: prefer clsx (or a template literal) over cx from class-variance-authority for simple class merging.

cx from class-variance-authority works correctly here, but class-variance-authority is primarily designed for variant-driven class generation via cva(). Pulling in cx just to join two CSS module classes conflates a utility concern with a design-system concern. A clsx import (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

📥 Commits

Reviewing files that changed from the base of the PR and between 29b06e1 and d13169a.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • packages/chronicle/package.json
  • packages/chronicle/src/themes/paper/Page.module.css
  • packages/chronicle/src/themes/paper/Page.tsx
  • packages/chronicle/src/themes/paper/Skeleton.tsx

@rsbh rsbh merged commit 9e86794 into main May 8, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants