You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/05/01 00:21:26 UTC
[GitHub] [superset] rusackas commented on a change in pull request #14184: refactor(navbar): migrate Bootstrap navbar to AntD menus
rusackas commented on a change in pull request #14184:
URL: https://github.com/apache/superset/pull/14184#discussion_r624318575
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -151,147 +258,116 @@ const StyledHeader = styled.header`
${({ theme }) => theme.gridUnit * 2}px;
}
}
+ .ant-menu-submenu-popup {
+ border-radius: 0px !important;
Review comment:
Check to see if we need all the !importants in these files.
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -92,16 +105,108 @@ const StyledHeader = styled.header`
flex-direction: column;
justify-content: center;
}
-
- .nav > li > a {
+ @media (max-width: 767px) {
+ .navbar-brand {
+ float: none;
+ }
+ }
+ .ant-menu-horizontal .ant-menu-item {
+ height: 100%;
+ line-height: inherit;
+ }
+ .ant-menu > .ant-menu-item > a {
padding: ${({ theme }) => theme.gridUnit * 4}px;
}
+ @media (max-width: 767px) {
+ .ant-menu > .ant-menu-item > a {
+ padding: 0px;
+ }
+ .main-nav .ant-menu-submenu-title > svg:nth-child(1) {
+ display: none;
+ }
+ .ant-menu-item-active > a {
+ &:hover {
+ color: ${({ theme }) => theme.colors.primary.base} !important;
+ background-color: transparent !important;
+ }
+ }
+ }
.dropdown-header {
text-transform: uppercase;
padding-left: 12px;
}
+ .ant-menu-horizontal:not(.ant-menu-dark) > .ant-menu-submenu,
+ .ant-menu-horizontal:not(.ant-menu-dark) > .ant-menu-item {
+ margin: 0px;
+ &:hover {
+ border-bottom: none;
+ }
+ }
+ .ant-menu-horizontal > .ant-menu-item,
+ .ant-menu-horizontal > .ant-menu-submenu {
+ vertical-align: inherit;
+ &:hover {
+ color: ${({ theme }) => theme.colors.grayscale.dark1};
+ }
+ }
+ .navbar-right {
+ border: none;
+ }
+ .ant-menu-horizontal {
+ line-height: 51px;
+ border: none;
+ .ant-menu-submenu-open,
+ .ant-menu-submenu-active,
+ .ant-menu-submenu,
+ .ant-menu-submenu-horizontal {
+ color: ${({ theme }) => theme.colors.grayscale.dark1};
+ border-bottom: none;
+ }
- .navbar-inverse .navbar-nav li a {
+ .ant-menu-submenu-open,
+ .ant-menu-submenu-active {
+ background-color: ${({ theme }) => theme.colors.primary.light5};
+ .ant-menu-submenu-title {
+ color: ${({ theme }) => theme.colors.grayscale.dark1};
+ background-color: ${({ theme }) => theme.colors.primary.light5};
+ border-bottom: none;
+ margin: 0;
+ &:after {
+ opacity: 1;
+ width: 99%;
Review comment:
Check to see if this is needed... normally I'd expect to see 100% or something like calc(100% - 1px);
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -151,147 +258,116 @@ const StyledHeader = styled.header`
${({ theme }) => theme.gridUnit * 2}px;
}
}
+ .ant-menu-submenu-popup {
+ border-radius: 0px !important;
+ background-color: red !important;
+ }
`;
+const { SubMenu } = DropdownMenu;
+
export function Menu({
data: { menu, brand, navbar_right: navbarRight, settings },
isFrontendRoute = () => false,
}: MenuProps) {
- const [dropdownOpen, setDropdownOpen] = useState(false);
+ const [showMenu, setMenu] = useState<MenuMode>('horizontal');
- return (
- <StyledHeader className="top" id="main-menu">
- <Navbar inverse fluid staticTop role="navigation">
- <Navbar.Header>
- <Navbar.Brand>
- <a className="navbar-brand" href={brand.path}>
- <img width={brand.width} src={brand.icon} alt={brand.alt} />
- </a>
- </Navbar.Brand>
- <Navbar.Toggle />
- </Navbar.Header>
- <Nav data-test="navbar-top">
- {menu.map((item, index) => {
- const props = {
- ...item,
- isFrontendRoute: isFrontendRoute(item.url),
- childs: item.childs?.map(c => {
- if (typeof c === 'string') {
- return c;
- }
+ useEffect(() => {
+ function handleResize() {
+ if (window.innerWidth <= 767) {
+ setMenu('inline');
+ } else setMenu('horizontal');
+ }
+ handleResize();
Review comment:
or, check out `useResizeDetector` from the Explore chart panel
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -92,16 +105,108 @@ const StyledHeader = styled.header`
flex-direction: column;
justify-content: center;
}
-
- .nav > li > a {
+ @media (max-width: 767px) {
+ .navbar-brand {
+ float: none;
+ }
+ }
+ .ant-menu-horizontal .ant-menu-item {
+ height: 100%;
+ line-height: inherit;
+ }
+ .ant-menu > .ant-menu-item > a {
padding: ${({ theme }) => theme.gridUnit * 4}px;
}
+ @media (max-width: 767px) {
+ .ant-menu > .ant-menu-item > a {
+ padding: 0px;
+ }
+ .main-nav .ant-menu-submenu-title > svg:nth-child(1) {
+ display: none;
+ }
+ .ant-menu-item-active > a {
+ &:hover {
+ color: ${({ theme }) => theme.colors.primary.base} !important;
+ background-color: transparent !important;
+ }
+ }
+ }
.dropdown-header {
text-transform: uppercase;
padding-left: 12px;
}
+ .ant-menu-horizontal:not(.ant-menu-dark) > .ant-menu-submenu,
+ .ant-menu-horizontal:not(.ant-menu-dark) > .ant-menu-item {
+ margin: 0px;
+ &:hover {
+ border-bottom: none;
+ }
+ }
+ .ant-menu-horizontal > .ant-menu-item,
+ .ant-menu-horizontal > .ant-menu-submenu {
+ vertical-align: inherit;
+ &:hover {
+ color: ${({ theme }) => theme.colors.grayscale.dark1};
+ }
+ }
+ .navbar-right {
+ border: none;
+ }
+ .ant-menu-horizontal {
+ line-height: 51px;
Review comment:
If this is for vertical centering, _maybe_ we can do something with flexbox to vertically center the content
##########
File path: superset-frontend/src/components/Menu/Menu.tsx
##########
@@ -151,147 +258,116 @@ const StyledHeader = styled.header`
${({ theme }) => theme.gridUnit * 2}px;
}
}
+ .ant-menu-submenu-popup {
+ border-radius: 0px !important;
+ background-color: red !important;
+ }
`;
+const { SubMenu } = DropdownMenu;
+
export function Menu({
data: { menu, brand, navbar_right: navbarRight, settings },
isFrontendRoute = () => false,
}: MenuProps) {
- const [dropdownOpen, setDropdownOpen] = useState(false);
+ const [showMenu, setMenu] = useState<MenuMode>('horizontal');
- return (
- <StyledHeader className="top" id="main-menu">
- <Navbar inverse fluid staticTop role="navigation">
- <Navbar.Header>
- <Navbar.Brand>
- <a className="navbar-brand" href={brand.path}>
- <img width={brand.width} src={brand.icon} alt={brand.alt} />
- </a>
- </Navbar.Brand>
- <Navbar.Toggle />
- </Navbar.Header>
- <Nav data-test="navbar-top">
- {menu.map((item, index) => {
- const props = {
- ...item,
- isFrontendRoute: isFrontendRoute(item.url),
- childs: item.childs?.map(c => {
- if (typeof c === 'string') {
- return c;
- }
+ useEffect(() => {
+ function handleResize() {
+ if (window.innerWidth <= 767) {
+ setMenu('inline');
+ } else setMenu('horizontal');
+ }
+ handleResize();
Review comment:
debounce this... maybe with lodash, or @suddjian might have something to leverage...
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org