From 3e35ddd60970f5b605df0b19a11b0698bab78ecb Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Wed, 4 Nov 2020 23:32:38 +0100 Subject: [PATCH] refactor: Replace react-bootstrap MenuItems with Antd Menu (#11555) * Remove MenuItem from SubMenu * Fix tests * Refactor PopoverDropdown * Refactor Button * Remove redundant Menu import --- .../javascripts/components/SubMenu_spec.jsx | 6 +- .../components/gridComponents/Header_spec.jsx | 2 +- .../gridComponents/Markdown_spec.jsx | 2 +- .../CRUD/welcome/DashboardTable_spec.tsx | 4 +- .../views/CRUD/welcome/SavedQueries_spec.tsx | 4 +- .../src/components/Button/index.tsx | 18 +++--- .../src/components/Menu/SubMenu.tsx | 18 +++--- .../components/menu/PopoverDropdown.jsx | 62 ++++++++++++++----- 8 files changed, 78 insertions(+), 38 deletions(-) diff --git a/superset-frontend/spec/javascripts/components/SubMenu_spec.jsx b/superset-frontend/spec/javascripts/components/SubMenu_spec.jsx index ab0020d3c7..46259328f8 100644 --- a/superset-frontend/spec/javascripts/components/SubMenu_spec.jsx +++ b/superset-frontend/spec/javascripts/components/SubMenu_spec.jsx @@ -19,7 +19,7 @@ import React from 'react'; import { Link } from 'react-router-dom'; import { shallow } from 'enzyme'; -import { Navbar, MenuItem } from 'react-bootstrap'; +import { Navbar } from 'react-bootstrap'; import SubMenu from 'src/components/Menu/SubMenu'; const defaultProps = { @@ -66,7 +66,7 @@ describe('SubMenu', () => { }); it('renders 3 MenuItems (when usesRouter === false)', () => { - expect(wrapper.find(MenuItem)).toHaveLength(3); + expect(wrapper.find('li')).toHaveLength(3); }); it('renders the menu title', () => { @@ -83,7 +83,7 @@ describe('SubMenu', () => { expect(routerWrapper.find(Link)).toExist(); expect(routerWrapper.find(Link)).toHaveLength(2); - expect(routerWrapper.find(MenuItem)).toHaveLength(1); + expect(routerWrapper.find('li.no-router')).toHaveLength(1); }); it('renders buttons in the right nav of the submenu', () => { diff --git a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Header_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Header_spec.jsx index 3e8b229bdf..3e057d1974 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Header_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Header_spec.jsx @@ -18,7 +18,7 @@ */ import React from 'react'; import { Provider } from 'react-redux'; -import { mount } from 'enzyme'; +import { styledMount as mount } from 'spec/helpers/theming'; import sinon from 'sinon'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; diff --git a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Markdown_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Markdown_spec.jsx index ad82b52f82..343bcf9406 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Markdown_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Markdown_spec.jsx @@ -18,7 +18,7 @@ */ import { Provider } from 'react-redux'; import React from 'react'; -import { mount } from 'enzyme'; +import { styledMount as mount } from 'spec/helpers/theming'; import sinon from 'sinon'; import ReactMarkdown from 'react-markdown'; diff --git a/superset-frontend/spec/javascripts/views/CRUD/welcome/DashboardTable_spec.tsx b/superset-frontend/spec/javascripts/views/CRUD/welcome/DashboardTable_spec.tsx index e7f28f3f9c..1ee8d06127 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/welcome/DashboardTable_spec.tsx +++ b/superset-frontend/spec/javascripts/views/CRUD/welcome/DashboardTable_spec.tsx @@ -69,10 +69,10 @@ describe('DashboardTable', () => { it('render a submenu with clickable tabs and buttons', async () => { expect(wrapper.find(SubMenu)).toExist(); - expect(wrapper.find('MenuItem')).toHaveLength(2); + expect(wrapper.find('li')).toHaveLength(2); expect(wrapper.find('Button')).toHaveLength(4); act(() => { - wrapper.find('MenuItem').at(1).simulate('click'); + wrapper.find('li').at(1).simulate('click'); }); await waitForComponentToPaint(wrapper); expect(fetchMock.calls(/dashboard\/\?q/)).toHaveLength(1); diff --git a/superset-frontend/spec/javascripts/views/CRUD/welcome/SavedQueries_spec.tsx b/superset-frontend/spec/javascripts/views/CRUD/welcome/SavedQueries_spec.tsx index 2670482d7b..2e290158a7 100644 --- a/superset-frontend/spec/javascripts/views/CRUD/welcome/SavedQueries_spec.tsx +++ b/superset-frontend/spec/javascripts/views/CRUD/welcome/SavedQueries_spec.tsx @@ -89,10 +89,10 @@ describe('SavedQueries', () => { it('it renders a submenu with clickable tables and buttons', async () => { expect(wrapper.find(SubMenu)).toExist(); - expect(wrapper.find('MenuItem')).toHaveLength(2); + expect(wrapper.find('li')).toHaveLength(2); expect(wrapper.find('button')).toHaveLength(2); act(() => { - wrapper.find('MenuItem').at(1).simulate('click'); + wrapper.find('li').at(1).simulate('click'); }); await waitForComponentToPaint(wrapper); diff --git a/superset-frontend/src/components/Button/index.tsx b/superset-frontend/src/components/Button/index.tsx index 53042dd184..f9fa66d1ad 100644 --- a/superset-frontend/src/components/Button/index.tsx +++ b/superset-frontend/src/components/Button/index.tsx @@ -24,9 +24,9 @@ import { Button as BootstrapButton, Tooltip, OverlayTrigger, - MenuItem, } from 'react-bootstrap'; import { styled } from '@superset-ui/core'; +import { Menu } from 'src/common/components'; export type OnClickHandler = React.MouseEventHandler; @@ -285,12 +285,16 @@ export default function Button({ {children} ); diff --git a/superset-frontend/src/components/Menu/SubMenu.tsx b/superset-frontend/src/components/Menu/SubMenu.tsx index 8f0e171a2c..cc6914d484 100644 --- a/superset-frontend/src/components/Menu/SubMenu.tsx +++ b/superset-frontend/src/components/Menu/SubMenu.tsx @@ -19,7 +19,8 @@ import React, { ReactNode } from 'react'; import { Link, useHistory } from 'react-router-dom'; import { styled } from '@superset-ui/core'; -import { Nav, Navbar, MenuItem } from 'react-bootstrap'; +import cx from 'classnames'; +import { Nav, Navbar } from 'react-bootstrap'; import Button, { OnClickHandler } from 'src/components/Button'; const StyledHeader = styled.header` @@ -139,15 +140,16 @@ const SubMenu: React.FunctionComponent = props => { } return ( - - {tab.label} - + + {tab.label} + + ); })} diff --git a/superset-frontend/src/dashboard/components/menu/PopoverDropdown.jsx b/superset-frontend/src/dashboard/components/menu/PopoverDropdown.jsx index 383ab2ed7f..daad642906 100644 --- a/superset-frontend/src/dashboard/components/menu/PopoverDropdown.jsx +++ b/superset-frontend/src/dashboard/components/menu/PopoverDropdown.jsx @@ -18,7 +18,10 @@ */ import React from 'react'; import PropTypes from 'prop-types'; -import { DropdownButton, MenuItem } from 'react-bootstrap'; +import cx from 'classnames'; +import { styled } from '@superset-ui/core'; +import { DropdownButton } from 'react-bootstrap'; +import { Menu } from 'src/common/components'; const propTypes = { id: PropTypes.string.isRequired, @@ -42,14 +45,44 @@ const defaultProps = { ), }; +const MenuItem = styled(Menu.Item)` + &.ant-menu-item { + height: auto; + line-height: 1.4; + + padding-top: ${({ theme }) => theme.gridUnit}px; + padding-bottom: ${({ theme }) => theme.gridUnit}px; + + margin-top: 0; + margin-bottom: 0; + + &:not(:last-child) { + margin-bottom: 0; + } + + &:hover { + background: ${({ theme }) => theme.colors.grayscale.light3}; + } + + &.active { + font-weight: ${({ theme }) => theme.typography.weights.bold}; + background: ${({ theme }) => theme.colors.grayscale.light2}; + } + } + + &.ant-menu-item-selected { + color: unset; + } +`; + class PopoverDropdown extends React.PureComponent { constructor(props) { super(props); this.handleSelect = this.handleSelect.bind(this); } - handleSelect(nextValue) { - this.props.onChange(nextValue); + handleSelect({ key }) { + this.props.onChange(key); } render() { @@ -62,17 +95,18 @@ class PopoverDropdown extends React.PureComponent { title={renderButton(selected)} className="popover-dropdown" > - {options.map(option => ( - - {renderOption(option)} - - ))} + + {options.map(option => ( + + {renderOption(option)} + + ))} + ); }