Re: [PR] chore(Alert): Migrate Alert component to Ant Design V5 [superset]

["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