You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gabotorresruiz (via GitHub)" <gi...@apache.org> on 2025/10/06 21:41:57 UTC
[PR] fix(theming): solve modal dark theme issues + styling and code improvements [superset]
gabotorresruiz opened a new pull request, #35539:
URL: https://github.com/apache/superset/pull/35539
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes dark theme issues where imperative Modal APIs (`Modal.confirm`, `Modal.error`, `Modal.warning`) render to `document.body` without theme context, causing white backgrounds in `dark mode`.
**Changes:**
- Created reusable `ConfirmModal` component and `useConfirmModal` hook in `@superset-ui/core`
- Replaced `Modal.confirm` with `useConfirmModal` in:
- `ControlPanelsContainer` (X-axis filter suggestion)
- `ResultSet` (CSV download limit warning)
- Refactored `CopyStyledButton` in `ResultSet` to use themed hover color (`theme.colorPrimary`)
- `ThemeList` (system default confirmations - refactored for consistency)
- Replaced `Modal.error` with `addDangerToast` in:
- Dashboard `PropertiesModal` (error handling)
- Explore `PropertiesModal` (error handling)
- Implemented `Alert` footer pattern in `EmbeddedModal` (deactivation confirmation)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
#### Before:
<img width="458" height="220" alt="Screenshot 2025-10-03 at 13 36 36" src="https://github.com/user-attachments/assets/dd0e8fb3-17ea-4259-9fc8-6bd826da4cf7" />
<img width="534" height="661" alt="Screenshot 2025-10-03 at 13 17 16" src="https://github.com/user-attachments/assets/89f01dc0-e91e-4457-9c97-7a8bd5ca499f" />
<img width="527" height="381" alt="Screenshot 2025-10-03 at 13 12 53" src="https://github.com/user-attachments/assets/72e01190-0421-464c-b43c-bbc36c2b6028" />
<img width="535" height="289" alt="Screenshot 2025-10-03 at 13 05 09" src="https://github.com/user-attachments/assets/2b1ec45b-5dd9-4766-af3f-6c61920aa542" />
<img width="675" height="472" alt="Screenshot 2025-10-03 at 13 02 13" src="https://github.com/user-attachments/assets/1d9a288f-983c-465b-89ad-7f98f956e90c" />
-------------------------------------------------------------------------------------------
#### After:
<img width="613" height="238" alt="image" src="https://github.com/user-attachments/assets/f8cdaadb-9f1a-4750-8478-e0f98d021fa5" />
<img width="1512" height="826" alt="Screenshot 2025-10-06 at 11 21 37" src="https://github.com/user-attachments/assets/cadb194e-4043-48b6-ad8d-ba2c40fd0437" />
<img width="648" height="316" alt="image" src="https://github.com/user-attachments/assets/05772273-3d53-47db-843c-7d5917f1d8fd" />
<img width="639" height="479" alt="Screenshot 2025-10-06 at 13 56 40" src="https://github.com/user-attachments/assets/3675ae9d-536d-431f-96b7-08587de46f35" />
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Enable dark theme in Superset
2. **Test ControlPanelsContainer (X-axis filter):**
- Go to Explore view
- Choose a dataset (e.g.: channels)
- Select a chart type with X-axis (e.g.: Line Chart)
- Choose an X-axis column not in filters (e.g.: topic__last_set)
- Verify confirmation modal appears with dark theme
3. **Test ResultSet (CSV download warning):**
- Go to `SQL Lab`
- Run a query with LIMIT applied
- Click `Download CSV`
- Verify warning modal appears with dark theme
- Verify button hover state uses theme color
5. **Test EmbeddedModal (deactivation):**
- Enable embedded dashboards in `superset/config.py`
- Go to a dashboard
- Open embed settings
- Activate the embedding
- Open embed settings again
- Click "Deactivate"
- Verify Alert component appears inline (not a new modal)
7. **Test PropertiesModal errors:**
- Trigger validation errors in dashboard/chart properties
- Verify error toasts appear instead of modal dialogs
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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(theming): solve modal dark theme issues + styling and code improvements [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35539:
URL: https://github.com/apache/superset/pull/35539#issuecomment-3374320299
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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(theming): solve modal dark theme issues + styling and code improvements [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #35539:
URL: https://github.com/apache/superset/pull/35539#issuecomment-3374320100
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](755f6012-cd61-4b15-9127-28633df927d0)
--
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(theming): solve modal dark theme issues + styling and code improvements [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #35539:
URL: https://github.com/apache/superset/pull/35539
--
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(theming): solve modal dark theme issues + styling and code improvements [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35539:
URL: https://github.com/apache/superset/pull/35539#discussion_r2408969238
##########
superset-frontend/packages/superset-ui-core/src/components/ConfirmModal/index.tsx:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t, styled } from '@superset-ui/core';
+import { Icons, Modal, Typography, Button } from '@superset-ui/core/components';
+import type { FC, ReactElement, ReactNode } from 'react';
+
+const IconWrapper = styled.span`
+ margin-right: ${({ theme }) => theme.sizeUnit * 2}px;
+`;
+
+export type ConfirmModalProps = {
+ show: boolean;
+ onHide: () => void;
+ onConfirm: () => void;
+ title: string;
+ body: string | ReactNode;
+ confirmText?: string;
+ cancelText?: string;
+ confirmButtonStyle?: 'primary' | 'danger' | 'dashed';
+ icon?: ReactNode;
+ loading?: boolean;
+};
+
+export const ConfirmModal: FC<ConfirmModalProps> = ({
+ show,
+ onHide,
+ onConfirm,
+ title,
+ body,
+ confirmText = t('Confirm'),
+ cancelText = t('Cancel'),
+ confirmButtonStyle = 'primary',
+ icon = <Icons.QuestionCircleOutlined iconSize="m" />,
+ loading = false,
+}: ConfirmModalProps): ReactElement => (
Review Comment:
### High Component Coupling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component is tightly coupled with the specific Modal and Button implementations from @superset-ui/core/components, making it difficult to adapt or modify the underlying implementation.
###### Why this matters
This tight coupling makes it challenging to switch UI libraries, test the component in isolation, or create variations of the modal with different underlying implementations.
###### Suggested change ∙ *Feature Preview*
Create an abstraction layer by introducing an interface for the modal behavior and extracting the UI-specific implementation:
```typescript
interface IModalImplementation {
show: boolean;
onClose: () => void;
renderContent: (props: ModalContentProps) => ReactNode;
}
export const ConfirmModal: FC<ConfirmModalProps & { implementation?: IModalImplementation }> = ({
implementation = DefaultModalImplementation,
...props
}) => implementation.renderContent(props);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f56ae94b-ec51-4b96-92fc-cf465ba70e9e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f56ae94b-ec51-4b96-92fc-cf465ba70e9e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f56ae94b-ec51-4b96-92fc-cf465ba70e9e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f56ae94b-ec51-4b96-92fc-cf465ba70e9e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f56ae94b-ec51-4b96-92fc-cf465ba70e9e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a9344f27-14b0-4b3e-a35c-9df1f5bc04df -->
[](a9344f27-14b0-4b3e-a35c-9df1f5bc04df)
##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -327,6 +329,22 @@ const ResultSet = ({
user?.roles,
);
+ const handleDownloadCsv = () => {
+ logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {});
+ if (limitingFactor === LimitingFactor.Dropdown && limit === rowsCount) {
+ showConfirm({
+ title: t('Download is on the way'),
+ body: t(
+ 'Downloading %(rows)s rows based on the LIMIT configuration. If you want the entire result set, you need to adjust the LIMIT.',
+ { rows: rowsCount.toLocaleString() },
+ ),
+ onConfirm: () => {},
+ confirmText: t('OK'),
+ cancelText: t('Close'),
+ });
+ }
+ };
Review Comment:
### CSV download blocked by confirm modal <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The CSV download functionality is broken because the confirm modal blocks the download without actually triggering it.
###### Why this matters
When the limiting condition is met, the modal appears but the actual CSV download (via the href attribute) never executes because the modal intercepts the click event. Users will see the warning but won't get their CSV file.
###### Suggested change ∙ *Feature Preview*
Move the download logic inside the onConfirm callback or restructure to allow the download to proceed after showing the warning:
```typescript
const handleDownloadCsv = () => {
logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {});
if (limitingFactor === LimitingFactor.Dropdown && limit === rowsCount) {
showConfirm({
title: t('Download is on the way'),
body: t(
'Downloading %(rows)s rows based on the LIMIT configuration. If you want the entire result set, you need to adjust the LIMIT.',
{ rows: rowsCount.toLocaleString() },
),
onConfirm: () => {
// Trigger the actual download here
window.location.href = getExportCsvUrl(query.id);
},
confirmText: t('OK'),
cancelText: t('Close'),
});
return false; // Prevent default href behavior
}
// Allow normal href download for other cases
};
```
And remove the href from the button when the modal should be shown, or handle the download programmatically in both cases.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5da9a5a8-9bab-4f8b-8207-91917bcd5fe8/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5da9a5a8-9bab-4f8b-8207-91917bcd5fe8?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5da9a5a8-9bab-4f8b-8207-91917bcd5fe8?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5da9a5a8-9bab-4f8b-8207-91917bcd5fe8?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5da9a5a8-9bab-4f8b-8207-91917bcd5fe8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:68886c8d-2ee0-4e32-b536-f9d372a8a673 -->
[](68886c8d-2ee0-4e32-b536-f9d372a8a673)
##########
superset-frontend/packages/superset-ui-core/src/components/ConfirmModal/index.tsx:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t, styled } from '@superset-ui/core';
+import { Icons, Modal, Typography, Button } from '@superset-ui/core/components';
+import type { FC, ReactElement, ReactNode } from 'react';
+
+const IconWrapper = styled.span`
+ margin-right: ${({ theme }) => theme.sizeUnit * 2}px;
+`;
+
+export type ConfirmModalProps = {
+ show: boolean;
+ onHide: () => void;
+ onConfirm: () => void;
+ title: string;
+ body: string | ReactNode;
+ confirmText?: string;
+ cancelText?: string;
+ confirmButtonStyle?: 'primary' | 'danger' | 'dashed';
+ icon?: ReactNode;
+ loading?: boolean;
+};
+
+export const ConfirmModal: FC<ConfirmModalProps> = ({
+ show,
+ onHide,
+ onConfirm,
+ title,
+ body,
+ confirmText = t('Confirm'),
+ cancelText = t('Cancel'),
+ confirmButtonStyle = 'primary',
+ icon = <Icons.QuestionCircleOutlined iconSize="m" />,
+ loading = false,
+}: ConfirmModalProps): ReactElement => (
+ <Modal
+ centered
+ responsive
+ onHide={onHide}
+ show={show}
+ width="600px"
+ title={
+ <>
+ <IconWrapper>{icon}</IconWrapper>
+ {title}
+ </>
+ }
+ footer={
+ <>
+ <Button buttonStyle="secondary" onClick={onHide} disabled={loading}>
+ {cancelText}
+ </Button>
+ <Button
+ buttonStyle={confirmButtonStyle}
+ onClick={onConfirm}
+ loading={loading}
+ >
+ {confirmText}
+ </Button>
+ </>
+ }
+ >
+ <Typography.Text>{body}</Typography.Text>
Review Comment:
### Incorrect Typography.Text wrapping of ReactNode body <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The body prop is wrapped in Typography.Text even when it's a ReactNode, which can cause rendering issues or nested text elements when body contains complex JSX.
###### Why this matters
This will break when body contains non-text elements like buttons, links, or other components, and may create invalid HTML structure with nested text elements.
###### Suggested change ∙ *Feature Preview*
Conditionally render Typography.Text only for string bodies:
```tsx
{typeof body === 'string' ? <Typography.Text>{body}</Typography.Text> : body}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb9b8464-9e8f-4f33-8cce-838c123ada90/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb9b8464-9e8f-4f33-8cce-838c123ada90?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb9b8464-9e8f-4f33-8cce-838c123ada90?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb9b8464-9e8f-4f33-8cce-838c123ada90?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb9b8464-9e8f-4f33-8cce-838c123ada90)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0f996a0a-0f7f-4068-9701-e16742a29850 -->
[](0f996a0a-0f7f-4068-9701-e16742a29850)
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -133,17 +134,17 @@ function PropertiesModal({
return selectTags;
}, [tags.length]);
- function showError({ error, statusText, message }: any) {
- let errorText = error || statusText || t('An error has occurred');
- if (message === 'Forbidden') {
- errorText = t('You do not have permission to edit this chart');
- }
- Modal.error({
- title: t('Error'),
- content: errorText,
- okButtonProps: { danger: true, className: 'btn-danger' },
- });
- }
+ const showError = useCallback(
+ ({ error, statusText, message }: any) => {
+ let errorText = error || statusText || t('An error has occurred');
+ if (message === 'Forbidden') {
+ errorText = t('You do not have permission to edit this chart');
+ }
+
+ addDangerToast(errorText);
+ },
+ [addDangerToast],
+ );
Review Comment:
### Missing dependency in useCallback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The showError function is wrapped in useCallback with only addDangerToast as a dependency, but it also uses the t() translation function which is not included in the dependency array.
###### Why this matters
This creates a stale closure where the translation function reference may become outdated, potentially causing incorrect translations or runtime errors. It also violates React's exhaustive-deps rule which can lead to subtle bugs.
###### Suggested change ∙ *Feature Preview*
Add the t function to the dependency array:
```typescript
const showError = useCallback(
({ error, statusText, message }: any) => {
let errorText = error || statusText || t('An error has occurred');
if (message === 'Forbidden') {
errorText = t('You do not have permission to edit this chart');
}
addDangerToast(errorText);
},
[addDangerToast, t],
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0ed52f29-195a-4cc6-afcd-cb47abe63b05/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0ed52f29-195a-4cc6-afcd-cb47abe63b05?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0ed52f29-195a-4cc6-afcd-cb47abe63b05?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0ed52f29-195a-4cc6-afcd-cb47abe63b05?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0ed52f29-195a-4cc6-afcd-cb47abe63b05)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4857a747-55f4-44db-a56f-9747cfa174df -->
[](4857a747-55f4-44db-a56f-9747cfa174df)
##########
superset-frontend/packages/superset-ui-core/src/components/ConfirmModal/index.tsx:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t, styled } from '@superset-ui/core';
+import { Icons, Modal, Typography, Button } from '@superset-ui/core/components';
+import type { FC, ReactElement, ReactNode } from 'react';
+
+const IconWrapper = styled.span`
+ margin-right: ${({ theme }) => theme.sizeUnit * 2}px;
+`;
+
+export type ConfirmModalProps = {
+ show: boolean;
+ onHide: () => void;
+ onConfirm: () => void;
+ title: string;
+ body: string | ReactNode;
+ confirmText?: string;
+ cancelText?: string;
+ confirmButtonStyle?: 'primary' | 'danger' | 'dashed';
+ icon?: ReactNode;
+ loading?: boolean;
+};
+
+export const ConfirmModal: FC<ConfirmModalProps> = ({
+ show,
+ onHide,
+ onConfirm,
+ title,
+ body,
+ confirmText = t('Confirm'),
+ cancelText = t('Cancel'),
+ confirmButtonStyle = 'primary',
+ icon = <Icons.QuestionCircleOutlined iconSize="m" />,
Review Comment:
### Default icon recreated on every render <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Default icon is created on every render instead of being memoized or defined outside the component.
###### Why this matters
This causes unnecessary React element creation and reconciliation on each render, leading to potential performance degradation in applications with frequent re-renders.
###### Suggested change ∙ *Feature Preview*
Move the default icon outside the component or use React.memo/useMemo to prevent recreation:
```typescript
const DEFAULT_ICON = <Icons.QuestionCircleOutlined iconSize="m" />;
export const ConfirmModal: FC<ConfirmModalProps> = ({
// ... other props
icon = DEFAULT_ICON,
// ... rest
}) => (
// component body
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0f3caedb-2282-40c5-9c89-863d28330bc7/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0f3caedb-2282-40c5-9c89-863d28330bc7?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0f3caedb-2282-40c5-9c89-863d28330bc7?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0f3caedb-2282-40c5-9c89-863d28330bc7?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0f3caedb-2282-40c5-9c89-863d28330bc7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:835fb5b6-a622-427f-928c-f63488ffe450 -->
[](835fb5b6-a622-427f-928c-f63488ffe450)
--
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