[GitHub] [superset] eric-briscoe commented on a diff in pull request #21299: fix: DB connection modal connect bug

[GitBox <gi...@apache.org>]

eric-briscoe commented on code in PR #21299:
URL: https://github.com/apache/superset/pull/21299#discussion_r960983088


##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1509,6 +1511,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       show={show}
       title={<h4>{t('Connect a database')}</h4>}
       footer={renderModalFooter()}

Review Comment:
   @AAfghahi Do you want to be using the modalFooter variable here?



##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/styles.ts:
##########
@@ -156,7 +156,7 @@ export const antDModalStyles = (theme: SupersetTheme) => css`
   }
 
   .ant-modal-body {
-    height: ${theme.gridUnit * 180.5}px;
+    height: ${theme.gridUnit * 188}px;

Review Comment:
   A good practice is to define numbers as a const with a comment providing its purpose, then use the const.  Referred to commonly as a "magic number" because there is no context to what the value means.  Having a const with a comment will help future developers understand what this number represents, and how changing it will affect the rendering of the component. 
   
   I this case you would define a const outside of the style rendering block like:
   
   `const DESCRIPTIVE_NAME:number = 188 // Description of what this is for...`
   
   Then in the styling block use it like:
   `height: ${theme.gridUnit * DESCRIPTIVE_NAME}px;`
   
   I know you did not define this originally, but this is a simple opportunity to make code more readable for future devs :)
   



##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1509,6 +1511,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       show={show}
       title={<h4>{t('Connect a database')}</h4>}
       footer={renderModalFooter()}
+      hideFooter={renderModalFooter() ? false : true}

Review Comment:
   You can use simpler truthy/falsey statement here with the existing `modalFooter` variable:
   `hideFooter={!modalFooter}`
   



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