Fixing the AlertDialogPortal dark mode #470
Conversation
…that we want to add the AlertDialogPortal as a child of.
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
4-10: Consider enhancing type safety with explicit return types and type narrowing.The type definition is well-structured, but could benefit from some improvements:
type AlertDialogContextType = { isOpen: boolean; handleOpenChange: (open: boolean) => void; - floaterContext: FloatingContext; + floaterContext: Pick<FloatingContext, 'refs' | 'floatingStyles'>; - handleOverlayClick: () => void; + handleOverlayClick: () => void | undefined; rootClass: string; };This change:
- Narrows down FloatingContext to only required properties
- Makes the return type of handleOverlayClick explicit
src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx (1)
Line range hint
1-21: Architecture suggestion: Consider using data attributes.While using class names works, consider using data attributes (e.g.,
data-alert-dialog-root) instead of classes for targeting portal containers. This separates styling concerns from DOM targeting and prevents potential conflicts with CSS classes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx (1 hunks)
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx (1 hunks)
- styles/themes/components/alert-dialog.scss (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/components/ui/AlertDialog/shards/AlertDialogRoot.tsx
- styles/themes/components/alert-dialog.scss
🔇 Additional comments (3)
src/components/ui/AlertDialog/contexts/AlertDialogContext.tsx (1)
8-8: Verify dark mode class propagation.The
rootClassimplementation aligns with the PR objective of fixing dark mode styling. Let's verify its usage:Also applies to: 16-16
✅ Verification successful
Dark mode class propagation implementation is correct
The verification shows the
rootClassimplementation is working as intended:
AlertDialogRootgenerates the root class usingcustomClassSwitcher- The class is properly distributed through context to all child components (
Portal,Overlay,Content)- The component uses the RAD UI class naming convention (
rad-ui-alert-dialog) which supports theme inheritance- Dark mode classes can be applied either through the default class or a custom root class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how rootClass is being used in AlertDialogPortal and related components ast-grep --pattern 'rootClass = useContext(AlertDialogContext)' # Verify if there are any direct dark mode class references rg -l 'dark|theme' src/components/ui/AlertDialog/Length of output: 117
Script:
#!/bin/bash # Let's try a broader search for AlertDialogContext usage ast-grep --pattern 'useContext(AlertDialogContext)' # Search for rootClass usage in AlertDialog components rg 'rootClass' src/components/ui/AlertDialog/ -A 2 # Check AlertDialogPortal implementation fd AlertDialogPortal.tsx -t f -x cat {} # Look for dark mode related classes in the component rg -i 'className.*dark' src/components/ui/AlertDialog/Length of output: 4560
Script:
#!/bin/bash # Let's check how customClassSwitcher is used for dark mode rg -l "customClassSwitcher" src/core/ cat $(fd "customClassSwitcher" src/core/) # Check the actual class name generation rg "COMPONENT_NAME.*AlertDialog" src/components/ui/AlertDialog/ -A 2Length of output: 1162
src/components/ui/AlertDialog/shards/AlertDialogPortal.tsx (2)
1-1: LGTM: Import statements are appropriate.The added imports for
useContextandAlertDialogContextare necessary for the dark mode fix implementation.Also applies to: 3-3
11-12: Verify error handling for missing root element.While the implementation correctly retrieves the root element using the class name, we should consider the case where no element with the specified class exists.
Let's verify the usage of this class across the codebase:
| export const AlertDialogContext = createContext<AlertDialogContextType>({ | ||
| isOpen: false, | ||
| handleOpenChange: () => {}, | ||
| floaterContext: {} as FloatingContext, | ||
| rootClass: '', | ||
| handleOverlayClick: () => {}, | ||
| }); |
There was a problem hiding this comment.
Avoid unsafe type casting for FloatingContext default value.
The current implementation uses an unsafe type cast for the floaterContext default value. Consider providing a safer default implementation:
export const AlertDialogContext = createContext<AlertDialogContextType>({
isOpen: false,
handleOpenChange: () => {},
- floaterContext: {} as FloatingContext,
+ floaterContext: {
+ refs: { reference: null, floating: null, domReference: null },
+ floatingStyles: { position: 'fixed', top: '0', left: '0' }
+ },
rootClass: '',
handleOverlayClick: () => {},
});This provides a proper default structure that matches the expected FloatingContext shape, making it safer and more predictable.
Committable suggestion was skipped due to low confidence.
| return ( | ||
| <Floater.Portal> | ||
| <Floater.Portal root={rootElement || document.body}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a warning for fallback to document.body.
The fallback to document.body is correct, but it might be worth logging a warning in development when this happens as it indicates the dark mode won't work as expected.
- <Floater.Portal root={rootElement || document.body}>
+ <Floater.Portal
+ root={rootElement || (() => {
+ if (process.env.NODE_ENV === 'development') {
+ console.warn(
+ `AlertDialog: No element found with class "${rootClass}". ` +
+ 'Falling back to document.body. Dark mode styling may not work correctly.'
+ );
+ }
+ return document.body;
+ })()}>📝 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.
| <Floater.Portal root={rootElement || document.body}> | |
| <Floater.Portal | |
| root={rootElement || (() => { | |
| if (process.env.NODE_ENV === 'development') { | |
| console.warn( | |
| `AlertDialog: No element found with class "${rootClass}". ` + | |
| 'Falling back to document.body. Dark mode styling may not work correctly.' | |
| ); | |
| } | |
| return document.body; | |
| })()}> |
There was a problem hiding this comment.
should we accept this suggestion? @impirios
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -1,5 +1,18 @@ | |||
| import { createContext } from 'react'; | |||
| import { FloatingContext } from '@floating-ui/react'; | |||
There was a problem hiding this comment.
we can use the Floater component, you can add the items you need from the floating UI library that way, instead of directly importing - this makes sure we use only the API we strictly declare to use from the library
| z-index: 50; | ||
| background-color: rgba(0, 0, 0, 0.8); | ||
| .rad-ui-alert-dialog-overlay { | ||
| z-index: 50; |
There was a problem hiding this comment.
we should probably have a lint commit hook for CSS files no? 😁
There was a problem hiding this comment.
can keep it as a separate issue
Closes #468
We're using FloatingPortal from floating-ui/react as a way to add the dialog box children to the dom. by default it uses the document.body element and adds the HTML as a child to it so the dark theme classes didn't work on it
Solution
we will be using the rootClass from the
AlertDialogRootin AlertDialogPortal and get the element and pass it to the FloatingPortal.Summary by CodeRabbit