You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2025/09/18 22:06:24 UTC
[PR] fix(modals): restore App wrapper to fix Modal.confirm dark mode theming [superset]
eschutho opened a new pull request, #35198:
URL: https://github.com/apache/superset/pull/35198
## Summary
- Fixes confirmation dialogs (like "Set System Default Theme") appearing with white backgrounds in dark mode
- Restores App component wrapper in SupersetThemeProvider to provide theme context
- Modal.confirm dialogs now properly inherit dark mode theming
## Problem
Confirmation modals created with `Modal.confirm` were not respecting the current theme:
- ❌ White backgrounds in dark mode (as shown in screenshot)
- ❌ Confirmation dialogs rendered outside theme context
- ❌ App component wrapper was missing from theme provider
- ❌ Poor user experience with jarring white flashes
## Solution
**1. Restored App component wrapper:**
- Re-added `App` import from 'antd' in Theme.tsx
- Wrapped children with `<App>{children}</App>` in SupersetThemeProvider
- Provides theme context for imperatively created modals
**2. Enhanced themedConfirm function:**
- Maintained themedConfirm wrapper for Modal.confirm
- Added documentation explaining App wrapper dependency
- Ensures all Modal.confirm calls inherit current theme
**3. Theme inheritance:**
- Modal.confirm now automatically respects light/dark mode
- Consistent theming across all modal types
- No code changes needed for existing Modal.confirm usage
## Visual Impact
- ✅ Confirmation dialogs now use proper dark backgrounds in dark mode
- ✅ Consistent theming across all modal interactions
- ✅ No more white flashes when confirming actions
- ✅ Better visual consistency with app theme
## Technical Details
- Uses Ant Design's App component to provide theme context to portaled elements
- App component automatically injects theme for Modal.confirm, message, notification APIs
- Maintains backwards compatibility with existing modal usage
- Follows Ant Design v5 theming best practices
## Files Changed
- `superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx`
- `superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx`
## Testing
- ✅ Test Modal.confirm in both light and dark themes
- ✅ Verify confirmation dialogs use proper theme colors
- ✅ Check system theme change dialogs (main use case)
- ✅ Ensure no visual regressions with regular modals
🤖 Generated with [Claude Code](https://claude.ai/code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): restore App wrapper to fix Modal.confirm dark mode theming [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35198:
URL: https://github.com/apache/superset/pull/35198#discussion_r2361398752
##########
superset-frontend/src/pages/ThemeList/index.tsx:
##########
@@ -112,6 +112,9 @@ function ThemesList({
const [importingTheme, showImportModal] = useState<boolean>(false);
const [appliedThemeId, setAppliedThemeId] = useState<number | null>(null);
+ // Use Modal.useModal hook to ensure proper theming
+ const [modal, contextHolder] = Modal.useModal();
Review Comment:
### Inconsistent modal theming implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Modal.useModal hook is declared but the modal instance is not being used consistently throughout the component - only in the showThemeConfirmation function while other modal usage patterns remain unchanged.
###### Why this matters
This creates inconsistent theming behavior where some modals (those using the new modal.confirm) will have proper theming while others (like DeleteModal and ConfirmStatusChange) will still have theming issues, defeating the purpose of the change.
###### Suggested change ∙ *Feature Preview*
Ensure all modal instances in the component use the same theming approach. Since DeleteModal and ConfirmStatusChange are separate components that likely need their own theming fixes, the current implementation is actually correct for the Modal.confirm usage. However, verify that all Modal.confirm calls are replaced with modal.confirm - currently only the showThemeConfirmation function uses it correctly.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18726350-f0d7-4ddf-bb22-c5a7359a933a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18726350-f0d7-4ddf-bb22-c5a7359a933a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18726350-f0d7-4ddf-bb22-c5a7359a933a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18726350-f0d7-4ddf-bb22-c5a7359a933a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18726350-f0d7-4ddf-bb22-c5a7359a933a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3cb75775-6598-43ce-a5e1-a3425c5ca9c2 -->
[](3cb75775-6598-43ce-a5e1-a3425c5ca9c2)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): use Modal.useModal hook for proper dark mode theming [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35198:
URL: https://github.com/apache/superset/pull/35198#issuecomment-3366501362
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/18228564741) for [8b77348](https://github.com/apache/superset/commit/8b77348)
• **Environment:** http://35.91.196.36:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): use Modal.useModal hook for proper dark mode theming [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #35198:
URL: https://github.com/apache/superset/pull/35198
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): restore App wrapper to fix Modal.confirm dark mode theming [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35198:
URL: https://github.com/apache/superset/pull/35198#issuecomment-3310035739
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/17844137495) for [542949f](https://github.com/apache/superset/commit/542949f)
• **Environment:** http://34.215.184.202:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): restore App wrapper to fix Modal.confirm dark mode theming [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35198:
URL: https://github.com/apache/superset/pull/35198#discussion_r2361289192
##########
superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:
##########
@@ -368,9 +368,13 @@ const CustomModal = ({
};
CustomModal.displayName = 'Modal';
+// Theme-aware confirmation modal - now inherits theme through App wrapper in SupersetThemeProvider
+const themedConfirm = (props: Parameters<typeof AntdModal.confirm>[0]) =>
+ AntdModal.confirm(props);
Review Comment:
### Unnecessary function wrapper adds runtime overhead <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The themedConfirm wrapper creates an unnecessary function call layer without adding any functionality, as it directly passes through all props to AntdModal.confirm without modification.
###### Why this matters
This adds runtime overhead by creating an extra function call for every Modal.confirm invocation, while the comment suggests theme inheritance should happen through App wrapper in SupersetThemeProvider, not through this passthrough function.
###### Suggested change ∙ *Feature Preview*
Either remove the wrapper and use `AntdModal.confirm` directly, or implement actual theme-aware logic within the wrapper. If theme inheritance happens at the provider level as the comment suggests, the wrapper serves no purpose:
```typescript
export const Modal = Object.assign(CustomModal, {
error: AntdModal.error,
warning: AntdModal.warning,
confirm: AntdModal.confirm, // Direct reference
useModal: AntdModal.useModal,
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2938b91e-8ae9-449f-8b0c-a645b1944940/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2938b91e-8ae9-449f-8b0c-a645b1944940?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2938b91e-8ae9-449f-8b0c-a645b1944940?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2938b91e-8ae9-449f-8b0c-a645b1944940?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2938b91e-8ae9-449f-8b0c-a645b1944940)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:93e7ee9d-8861-4145-b4d1-8ee72fb66c1b -->
[](93e7ee9d-8861-4145-b4d1-8ee72fb66c1b)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): use Modal.useModal hook for proper dark mode theming [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35198:
URL: https://github.com/apache/superset/pull/35198#issuecomment-3366459424
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/18228564741) for [8b77348](https://github.com/apache/superset/commit/8b77348)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): restore App wrapper to fix Modal.confirm dark mode theming [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35198:
URL: https://github.com/apache/superset/pull/35198#discussion_r2361288297
##########
superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:
##########
@@ -368,9 +368,13 @@ const CustomModal = ({
};
CustomModal.displayName = 'Modal';
+// Theme-aware confirmation modal - now inherits theme through App wrapper in SupersetThemeProvider
Review Comment:
[nitpick] The `themedConfirm` function is a simple passthrough that doesn't add any theming logic. Consider adding a comment explaining that the actual theming is handled by the App wrapper in SupersetThemeProvider, or remove this wrapper if it's not adding value beyond documentation.
```suggestion
// The themedConfirm function is a simple passthrough to AntdModal.confirm.
// Actual theming is handled by the App wrapper in SupersetThemeProvider.
// This wrapper exists only for documentation and API consistency.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(modals): restore App wrapper to fix Modal.confirm dark mode theming [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35198:
URL: https://github.com/apache/superset/pull/35198#issuecomment-3310019101
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/17844137495) for [542949f](https://github.com/apache/superset/commit/542949f)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org