rusackas commented on a change in pull request #14184:
URL: https://github.com/apache/superset/pull/14184#discussion_r625912174
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -16,35 +16,65 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { ReactNode } from 'react';
+import React, { ReactNode, useState, useEffect } from 'react';
import { Link, useHistory } from 'react-router-dom';
import { styled } from '@superset-ui/core';
import cx from 'classnames';
-import { Nav, Navbar } from 'react-bootstrap';
+import { debounce } from 'lodash';
+import { Col, Row } from 'antd';
+import { Menu, MenuMode } from 'src/common/components';
import Button, { OnClickHandler } from 'src/components/Button';
-const StyledHeader = styled.header`
+const StyledHeader = styled.div`
margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
- .navbar {
- margin-bottom: 0;
- }
- .navbar-header .navbar-brand {
+ .header {
font-weight: ${({ theme }) => theme.typography.weights.bold};
margin-right: ${({ theme }) => theme.gridUnit * 3}px;
+ text-align: left;
+ font-size: 18px;
+ padding: ${({ theme }) => theme.gridUnit * 3}px;
+ display: inline-block;
+ line-height: ${({ theme }) => theme.gridUnit * 9}px;
}
- .navbar-right {
+ .nav-right {
display: flex;
align-items: center;
- padding: 8px 0;
+ padding: 14px 0;
+ margin-right: ${({ theme }) => theme.gridUnit * 3}px;
+ float: right;
+ }
+ .nav-right-collapse {
+ display: flex;
+ align-items: center;
+ padding: 14px 0;
margin-right: 0;
+ float: left;
+ padding-left: 10px;
+ }
+ .menu {
+ background-color: white;
+ .ant-menu-horizontal {
+ line-height: inherit;
+ .ant-menu-item {
+ &:hover {
+ border-bottom: none;
+ }
+ }
+ }
+ .ant-menu {
+ padding: ${({ theme }) => theme.gridUnit * 4}px 0px;
+ }
}
- .navbar-nav {
+
+ .ant-menu-horizontal:not(.ant-menu-dark) > .ant-menu-item {
Review comment:
Just curious why we need the `not(.ant-menu-dark)` bit here... should
this not be spaced the same if we did use the dark menu option?
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.test.tsx
##########
@@ -119,7 +119,7 @@ describe('ActivityTable', () => {
expect(chartCall).toHaveLength(1);
expect(dashboardCall).toHaveLength(1);
});
- it('show empty state if there is data', () => {
+ it('show empty state if there is no data', () => {
Review comment:
Hah... nice catch.
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -16,35 +16,65 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { ReactNode } from 'react';
+import React, { ReactNode, useState, useEffect } from 'react';
import { Link, useHistory } from 'react-router-dom';
import { styled } from '@superset-ui/core';
import cx from 'classnames';
-import { Nav, Navbar } from 'react-bootstrap';
+import { debounce } from 'lodash';
+import { Col, Row } from 'antd';
+import { Menu, MenuMode } from 'src/common/components';
import Button, { OnClickHandler } from 'src/components/Button';
-const StyledHeader = styled.header`
+const StyledHeader = styled.div`
margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
- .navbar {
- margin-bottom: 0;
- }
- .navbar-header .navbar-brand {
+ .header {
font-weight: ${({ theme }) => theme.typography.weights.bold};
margin-right: ${({ theme }) => theme.gridUnit * 3}px;
+ text-align: left;
+ font-size: 18px;
Review comment:
```suggestion
font-size: 18px;
```
Ask @mihir174 about this - the design system includes 16px (l) and 21px (xl)
font sizes... perhaps we should use one of these?
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -16,35 +16,65 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { ReactNode } from 'react';
+import React, { ReactNode, useState, useEffect } from 'react';
import { Link, useHistory } from 'react-router-dom';
import { styled } from '@superset-ui/core';
import cx from 'classnames';
-import { Nav, Navbar } from 'react-bootstrap';
+import { debounce } from 'lodash';
+import { Col, Row } from 'antd';
+import { Menu, MenuMode } from 'src/common/components';
import Button, { OnClickHandler } from 'src/components/Button';
-const StyledHeader = styled.header`
+const StyledHeader = styled.div`
margin-bottom: ${({ theme }) => theme.gridUnit * 4}px;
- .navbar {
- margin-bottom: 0;
- }
- .navbar-header .navbar-brand {
+ .header {
font-weight: ${({ theme }) => theme.typography.weights.bold};
margin-right: ${({ theme }) => theme.gridUnit * 3}px;
+ text-align: left;
+ font-size: 18px;
+ padding: ${({ theme }) => theme.gridUnit * 3}px;
+ display: inline-block;
+ line-height: ${({ theme }) => theme.gridUnit * 9}px;
}
- .navbar-right {
+ .nav-right {
display: flex;
align-items: center;
- padding: 8px 0;
+ padding: 14px 0;
Review comment:
There are a few of these specific-pixel paddings in here. They're
probably fine, but maybe @mihir174 would like to see what the next grid unit up
or down looks like, particularly in combination with the font size adjustment
mentioned above.
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -582,3 +582,35 @@ hr {
background-image: url('../images/icons/error_solid_small_red.svg')
!important;
background-position: -2px center !important;
}
+
+// Ant-d overrides since these are injected as inline styles and can't
+// be overrided in emotion
Review comment:
```suggestion
// AntD overrides since these are injected as inline styles and can't
// be overridden in emotion
```
Teensy nits, but the real question I'm wondering is if these same styles
couldn't be used in the main Menu component. These are all using !important,
which should still work in that context, I would think. Then the styles are all
consolidated with the component, and we're not growing the LESS codebase.
##########
File path: superset-frontend/stylesheets/superset.less
##########
@@ -582,3 +582,35 @@ hr {
background-image: url('../images/icons/error_solid_small_red.svg')
!important;
background-position: -2px center !important;
}
+
+// Ant-d overrides since these are injected as inline styles and can't
+// be overrided in emotion
+.ant-menu-submenu.ant-menu-submenu-popup.ant-menu.ant-menu-light.ant-menu-submenu-placement-bottomLeft
{
+ top: 51px !important;
+ margin-left: -2px !important;
+ @media (max-width: 767px) {
+ top: 269px !important;
+ }
+ & > .ant-menu {
+ border-radius: 0px !important;
+ }
+}
+
+.ant-menu-submenu.ant-menu-submenu-popup.ant-menu.ant-menu-light {
+ top: 51px !important;
+ border-radius: 0px !important;
+ @media (max-width: 767px) {
+ top: 269px !important;
Review comment:
This whole block of new styles contains a lot of highly specific pixel
measurements. Is this introducing fragility by pinning things in particular
locations? I just want to make sure that this doesn't look all broken if
something else changes, like the height of a logo or something.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
