Skip to content

feat: integrate theme support into Modal component for dynamic styling#71

Merged
Aunshon merged 1 commit intomainfrom
fix/modal-theme
Mar 16, 2026
Merged

feat: integrate theme support into Modal component for dynamic styling#71
Aunshon merged 1 commit intomainfrom
fix/modal-theme

Conversation

@Aunshon
Copy link
Collaborator

@Aunshon Aunshon commented Mar 16, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Fixed modals to properly respond to theme changes, ensuring consistent visual appearance across light and dark modes.

@coderabbitai
Copy link

coderabbitai bot commented Mar 16, 2026

📝 Walkthrough

Walkthrough

The modal component now integrates theme mode awareness by reading the current theme and dynamically applying the mode as a class name to the portal container. A useEffect hook ensures the portal's class remains synchronized when the theme mode changes.

Changes

Cohort / File(s) Summary
Modal Theme Synchronization
src/components/ui/modal.tsx
Added theme mode integration to modal portal: imports useTheme hook, applies mode to portal container className, and synchronizes portal class updates with theme changes via useEffect.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A modal dressed in theme's fine hue,
Now follows the mode through and through,
When darkness calls or daylight gleams,
The portal syncs with changing dreams! 🌙✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: integrating theme support into the Modal component for dynamic styling based on the useTheme hook.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/modal-theme
📝 Coding Plan
  • Generate coding plan for human review comments

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.

Copy link

@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)
src/components/ui/modal.tsx (1)

305-331: Consider adding an ESLint disable comment for the missing mode dependency.

The mode variable is used on line 315 but intentionally omitted from the dependency array. This is a valid design choice—the second useEffect (lines 334-338) handles mode synchronization without recreating the container, avoiding flicker. However, this will trigger an react-hooks/exhaustive-deps ESLint warning.

Adding an explicit disable comment documents this intentional pattern:

📝 Suggested comment addition
     return () => {
       document.body.style.overflow = originalOverflow;
       document.body.removeChild(container);

       if (previousActiveElement.current instanceof HTMLElement) {
         previousActiveElement.current.focus();
       }
     };
+    // eslint-disable-next-line react-hooks/exhaustive-deps -- mode changes handled by separate effect to avoid container recreation
   }, [open]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/ui/modal.tsx` around lines 305 - 331, Add an explicit ESLint
disable comment to the useEffect that creates the portal to document the
intentional omission of the mode dependency: locate the useEffect that
references mode and calls setPortalContainer/document.body manipulation (the
effect using previousActiveElement, container creation, originalOverflow) and
add a line disabling react-hooks/exhaustive-deps (e.g., an
eslint-disable-next-line react-hooks/exhaustive-deps comment immediately before
that useEffect) so the linter warning is suppressed while keeping the dependency
array as [open].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/components/ui/modal.tsx`:
- Around line 305-331: Add an explicit ESLint disable comment to the useEffect
that creates the portal to document the intentional omission of the mode
dependency: locate the useEffect that references mode and calls
setPortalContainer/document.body manipulation (the effect using
previousActiveElement, container creation, originalOverflow) and add a line
disabling react-hooks/exhaustive-deps (e.g., an eslint-disable-next-line
react-hooks/exhaustive-deps comment immediately before that useEffect) so the
linter warning is suppressed while keeping the dependency array as [open].

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ee1750a7-cdfc-4c52-ada5-a99ea8ddfa05

📥 Commits

Reviewing files that changed from the base of the PR and between e045017 and fefa607.

📒 Files selected for processing (1)
  • src/components/ui/modal.tsx

@Aunshon Aunshon merged commit 91b60e4 into main Mar 16, 2026
1 check passed
@Aunshon Aunshon deleted the fix/modal-theme branch March 16, 2026 06:41
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.

1 participant