feat: integrate theme support into Modal component for dynamic styling#71
feat: integrate theme support into Modal component for dynamic styling#71
Conversation
📝 WalkthroughWalkthroughThe 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 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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)
src/components/ui/modal.tsx (1)
305-331: Consider adding an ESLint disable comment for the missingmodedependency.The
modevariable is used on line 315 but intentionally omitted from the dependency array. This is a valid design choice—the seconduseEffect(lines 334-338) handles mode synchronization without recreating the container, avoiding flicker. However, this will trigger anreact-hooks/exhaustive-depsESLint 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
📒 Files selected for processing (1)
src/components/ui/modal.tsx
Summary by CodeRabbit