[GitHub] [superset] etr2460 commented on a diff in pull request #19586: fix: Navbar styles and Welcome page text

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

etr2460 commented on code in PR #19586:
URL: https://github.com/apache/superset/pull/19586#discussion_r845560092


##########
superset-frontend/src/views/CRUD/welcome/Welcome.tsx:
##########
@@ -113,23 +113,31 @@ const WelcomeContainer = styled.div`
 `;
 
 const WelcomeNav = styled.div`
-  height: 50px;
-  background-color: white;
-  .navbar-brand {
-    margin-left: ${({ theme }) => theme.gridUnit * 2}px;
-    font-weight: ${({ theme }) => theme.typography.weights.bold};
-  }
-  .switch {
-    float: right;
-    margin: ${({ theme }) => theme.gridUnit * 5}px;
-    display: flex;
-    flex-direction: row;
-    span {
-      display: block;
-      margin: ${({ theme }) => theme.gridUnit * 1}px;
-      line-height: 1;
+  ${({ theme }) => `
+    height: 50px;
+    background-color: ${theme.colors.grayscale.light5};
+    & > span {

Review Comment:
   can we give this a class name instead? this is a bit fragile



##########
superset-frontend/src/views/CRUD/welcome/Welcome.tsx:
##########
@@ -113,23 +113,31 @@ const WelcomeContainer = styled.div`
 `;
 
 const WelcomeNav = styled.div`
-  height: 50px;
-  background-color: white;
-  .navbar-brand {
-    margin-left: ${({ theme }) => theme.gridUnit * 2}px;
-    font-weight: ${({ theme }) => theme.typography.weights.bold};
-  }
-  .switch {
-    float: right;
-    margin: ${({ theme }) => theme.gridUnit * 5}px;
-    display: flex;
-    flex-direction: row;
-    span {
-      display: block;
-      margin: ${({ theme }) => theme.gridUnit * 1}px;
-      line-height: 1;
+  ${({ theme }) => `
+    height: 50px;
+    background-color: ${theme.colors.grayscale.light5};
+    & > span {
+      display: flex;
+      float: left;

Review Comment:
   why do we need float on this if we're displaying flex?



##########
superset-frontend/src/views/CRUD/welcome/Welcome.tsx:
##########
@@ -275,7 +283,7 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
   return (
     <WelcomeContainer>
       <WelcomeNav>
-        <span className="navbar-brand">Home</span>
+        <span>Home</span>

Review Comment:
   should this be an `h1` or an `h2` since it's the title of the page?



##########
superset-frontend/src/views/CRUD/welcome/Welcome.tsx:
##########
@@ -113,23 +113,31 @@ const WelcomeContainer = styled.div`
 `;
 
 const WelcomeNav = styled.div`
-  height: 50px;
-  background-color: white;
-  .navbar-brand {
-    margin-left: ${({ theme }) => theme.gridUnit * 2}px;
-    font-weight: ${({ theme }) => theme.typography.weights.bold};
-  }
-  .switch {
-    float: right;
-    margin: ${({ theme }) => theme.gridUnit * 5}px;
-    display: flex;
-    flex-direction: row;
-    span {
-      display: block;
-      margin: ${({ theme }) => theme.gridUnit * 1}px;
-      line-height: 1;
+  ${({ theme }) => `
+    height: 50px;
+    background-color: ${theme.colors.grayscale.light5};
+    & > span {
+      display: flex;
+      float: left;
+      height: 50px;

Review Comment:
   should this be a multiple of gridUnit?



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