[GitHub] [superset] rusackas commented on a change in pull request #19049: chore: Remove logo forced width

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

rusackas commented on a change in pull request #19049:
URL: https://github.com/apache/superset/pull/19049#discussion_r822833453



##########
File path: superset/config.py
##########
@@ -236,7 +236,6 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
 
 # Specify the App icon
 APP_ICON = "/static/assets/images/superset-logo-horiz.png"
-APP_ICON_WIDTH = 126

Review comment:
       If we do this, I think we should tag the PR with the `2.0` label and the `risk:breaking-change` label, add it to the 2.0 project board, and update UPDATING.MD as you've done here. 
   
   Or, as noted elsewhere, we COULD use that value in the CSS, to make it a little more configurable. If we want to keep that complexity and avoid the breaking change, that is.

##########
File path: superset-frontend/src/views/components/Menu.tsx
##########
@@ -97,6 +96,17 @@ const StyledHeader = styled.header`
     display: flex;
     flex-direction: column;
     justify-content: center;
+    min-height: ${({ theme }) => `${theme.gridUnit * 12.5}px`};

Review comment:
       Maybe just 12? Or lucky 13? Trying to avoid fractional gridUnits wherever possible, as usual.

##########
File path: superset-frontend/src/views/components/Menu.tsx
##########
@@ -97,6 +96,17 @@ const StyledHeader = styled.header`
     display: flex;
     flex-direction: column;
     justify-content: center;
+    min-height: ${({ theme }) => `${theme.gridUnit * 12.5}px`};
+    padding: ${({ theme }) =>
+      `${theme.gridUnit}px ${theme.gridUnit * 2}px ${theme.gridUnit}px ${
+        theme.gridUnit * 4
+      }px`};
+    max-width: ${({ theme }) => `${theme.gridUnit * 37}px`};

Review comment:
       I'm on the fence about this... 37 seems like a pretty arbitrary number. We _could_ use the `APP_ICON_WIDTH` here, I suppose, as much as I'd like to remove 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