You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "LevisNgigi (via GitHub)" <gi...@apache.org> on 2024/11/26 17:23:16 UTC
[PR] Upgrade alert antd v5 [superset]
LevisNgigi opened a new pull request, #31168:
URL: https://github.com/apache/superset/pull/31168
<!---
Upgrade Alert component from antdesign4 to antdesign5
-->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Before -->
<!---After--->
### TESTING INSTRUCTIONS
<!--- Trigger an alert to Verify the appearance of alerts throughout the application. -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ x ] Has associated issue:Fixes #30669
- [ ] Required feature flags:
- [ ] 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] chore(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31168:
URL: https://github.com/apache/superset/pull/31168#discussion_r1860434519
##########
superset-frontend/cypress-base/cypress/support/directories.ts:
##########
@@ -94,7 +94,7 @@ export const databasesPage = {
dbDropdown: '[class="ant-select-selection-search-input"]',
dbDropdownMenu: '.rc-virtual-list-holder-inner',
dbDropdownMenuItem: '[class="ant-select-item-option-content"]',
- infoAlert: '.ant-alert',
+ infoAlert: '.antd-v5-alert',
Review Comment:
```suggestion
infoAlert: '.antd5-alert',
```
##########
superset-frontend/src/components/Alert/Alert.stories.tsx:
##########
@@ -44,6 +44,7 @@ export const AlertGallery = () => (
type={type}
showIcon
closable
+ closeIcon={<span aria-label="close icon">x</span>} // Custom close icon
Review Comment:
Maybe remove `closeIcon` from here and create a separate Alert component with a custom close icon just to show this option in storybook
##########
superset-frontend/cypress-base/cypress/e2e/explore/visualizations/line.test.ts:
##########
@@ -71,7 +71,7 @@ describe('Visualization > Line', () => {
cy.get('#controlSections-tab-display').click();
cy.get('span').contains('Y Axis Bounds').scrollIntoView();
cy.get('input[placeholder="Min"]').type('-0.1', { delay: 100 });
- cy.get('.ant-alert-warning').should('not.exist');
+ cy.get('.antd-v5-alert-warning').should('not.exist');
Review Comment:
```suggestion
cy.get('.antd5-alert-warning').should('not.exist');
```
##########
superset-frontend/src/components/Alert/Alert.stories.tsx:
##########
@@ -82,6 +84,7 @@ InteractiveAlert.args = {
message: smallText,
description: bigText,
showIcon: true,
+ closeIcon: <span aria-label="close icon">x</span>,
Review Comment:
I think we do not need the `closeIcon` to be one of the storybook args as it accepts a ReactNode and it is hard to configure this in the storybook UI
##########
superset-frontend/cypress-base/cypress/support/directories.ts:
##########
@@ -166,7 +166,7 @@ export const sqlLabView = {
renderedTableHeader: '.ReactVirtualized__Table__headerRow',
renderedTableRow: '.ReactVirtualized__Table__row',
errorBody: '.error-body',
- alertMessage: '.ant-alert-message',
+ alertMessage: '.antd-v5-alert-message',
Review Comment:
```suggestion
alertMessage: '.antd5-alert-message',
```
##########
superset-frontend/cypress-base/cypress/e2e/explore/visualizations/line.test.ts:
##########
@@ -62,7 +62,7 @@ describe('Visualization > Line', () => {
'not.exist',
);
- cy.get('.ant-alert-warning').should('not.exist');
+ cy.get('.antd-v5-alert-warning').should('not.exist');
Review Comment:
```suggestion
cy.get('.antd5-alert-warning').should('not.exist');
```
##########
superset-frontend/src/components/Alert/index.tsx:
##########
@@ -56,32 +58,18 @@ export default function Alert(props: AlertProps) {
<AntdAlert
role="alert"
showIcon={showIcon}
- icon={<AlertIcon aria-label={`${type} icon`} />}
- closeText={closable && <Icons.XSmall aria-label="close icon" />}
- css={{
+ icon={showIcon && <AlertIcon aria-label={`${type} icon`} />}
+ closeIcon={closable && <Icons.XSmall aria-label="close icon" />}
+ message={children || 'Default message'}
+ description={description}
+ style={{
marginBottom: roomBelow ? gridUnit * 4 : 0,
padding: `${gridUnit * 2}px ${gridUnit * 3}px`,
alignItems: 'flex-start',
border: 0,
backgroundColor: baseColor.light2,
Review Comment:
I see you have removed a bunch of custom styles, which is good, but should we bring these in the component theme instead, including also those that you kept? You can see examples [here](https://github.com/apache/superset/pull/29786/files#diff-7c635266fc6affa5c3d678e1d8cf2a89451b283e168a1fb1f5b878342adb4a5aR72)
##########
superset-frontend/cypress-base/cypress/support/directories.ts:
##########
@@ -325,7 +325,7 @@ export const nativeFilters = {
confirmCancelButton: dataTestLocator(
'native-filter-modal-confirm-cancel-button',
),
- alertXUnsavedFilters: '.ant-alert-message',
+ alertXUnsavedFilters: '.antd-v5-alert-message',
Review Comment:
```suggestion
alertXUnsavedFilters: '.antd5-alert-message',
```
##########
superset-frontend/cypress-base/cypress/support/directories.ts:
##########
@@ -103,7 +103,7 @@ export const databasesPage = {
helperBottom: '.helper-bottom',
postgresDatabase: '[name="database"]',
dbInput: '[name="database_name"]',
- alertMessage: '.ant-alert-message',
+ alertMessage: '.antd-v5-alert-message',
Review Comment:
```suggestion
alertMessage: '.antd5-alert-message',
```
--
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] chore(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #31168:
URL: https://github.com/apache/superset/pull/31168#issuecomment-2504538985
@LevisNgigi should this PR be a refactor instead of a chore?
--
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] refactor(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #31168:
URL: https://github.com/apache/superset/pull/31168#issuecomment-2523862342
/testenv up
--
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] refactor(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on code in PR #31168:
URL: https://github.com/apache/superset/pull/31168#discussion_r1862577676
##########
superset-frontend/src/components/Alert/Alert.stories.tsx:
##########
@@ -44,6 +44,7 @@ export const AlertGallery = () => (
type={type}
showIcon
closable
+ closeIcon={<span aria-label="close icon">x</span>} // Custom close icon
Review Comment:
Thanks for the suggestion! I opted to showcase the closeIcon directly in the existing AlertGallery to keep things simple and avoid adding extra components. Since closeIcon isn't part of the Storybook args, I think this approach still effectively demonstrates its customization without adding another component.
--
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] refactor(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #31168:
URL: https://github.com/apache/superset/pull/31168
--
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] chore(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "LevisNgigi (via GitHub)" <gi...@apache.org>.
LevisNgigi commented on PR #31168:
URL: https://github.com/apache/superset/pull/31168#issuecomment-2504554192
> @LevisNgigi should this PR be a refactor instead of a chore?
@sadpandajoe Let me change it
--
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] refactor(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31168:
URL: https://github.com/apache/superset/pull/31168#discussion_r1866016284
##########
superset-frontend/src/components/Alert/index.tsx:
##########
@@ -55,33 +57,32 @@ export default function Alert(props: AlertProps) {
return (
<AntdAlert
role="alert"
+ aria-live={type === 'error' ? 'assertive' : 'polite'}
showIcon={showIcon}
- icon={<AlertIcon aria-label={`${type} icon`} />}
- closeText={closable && <Icons.XSmall aria-label="close icon" />}
- css={{
+ icon={
+ showIcon && (
+ <span
+ role="img"
+ aria-label={`${type} icon`}
+ style={{
+ color: baseColor.base,
+ }}
+ >
+ <AlertIcon />
+ </span>
+ )
+ }
+ closeIcon={closable && <Icons.XSmall aria-label="close icon" />}
+ message={children || 'Default message'}
+ description={description}
+ style={{
Review Comment:
Do we still need these styles here?
--
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] refactor(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31168:
URL: https://github.com/apache/superset/pull/31168#issuecomment-2523887146
@geido Ephemeral environment spinning up at http://null:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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] refactor(Alert): Migrate Alert component to Ant Design V5 [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31168:
URL: https://github.com/apache/superset/pull/31168#issuecomment-2523862659
@geido Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/12203687063).
--
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