refactor: Replace react-bootstrap Tabs with Antd Tabs in DashboardBui…

…lder (apache#11160) * Replace tabs in DashboardBuilder * Fix tests * Fix styling of anchor * Fix * Fix cypress test * Fix tests * Fix e2e tests * Use data-tests * Move tabs styles from superset.less to Emotion * Restyle tabs in DashboardBuilder * Test fix * Fix styling
auxten · Nov 20, 2020 · 9078762 · 9078762
1 parent ebf5c33
commit 9078762
Show file tree

Hide file tree

Showing 12 changed files with 194 additions and 289 deletions.
diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/tabs.test.js b/superset-frontend/cypress-base/cypress/integration/dashboard/tabs.test.js
@@ -97,15 +97,24 @@ describe('Dashboard tabs', () => {
 
     cy.get('[data-test="dashboard-component-tabs"]')
       .first()
-      .find('[data-test="nav-list"]')
-      .children()
+      .find('[data-test="nav-list"] .ant-tabs-nav-list > .ant-tabs-tab')
       .as('top-level-tabs');
 
-    cy.get('@top-level-tabs').first().click().should('have.class', 'active');
-    cy.get('@top-level-tabs').last().should('not.have.class', 'active');
+    cy.get('@top-level-tabs')
+      .first()
+      .click()
+      .should('have.class', 'ant-tabs-tab-active');
+    cy.get('@top-level-tabs')
+      .last()
+      .should('not.have.class', 'ant-tabs-tab-active');
 
-    cy.get('@top-level-tabs').last().click().should('have.class', 'active');
-    cy.get('@top-level-tabs').first().should('not.have.class', 'active');
+    cy.get('@top-level-tabs')
+      .last()
+      .click()
+      .should('have.class', 'ant-tabs-tab-active');
+    cy.get('@top-level-tabs')
+      .first()
+      .should('not.have.class', 'ant-tabs-tab-active');
   });
 
   it('should load charts when tab is visible', () => {
@@ -128,8 +137,7 @@ describe('Dashboard tabs', () => {
     // click row level tab, see 1 more chart
     cy.get('[data-test="dashboard-component-tabs"]')
       .last()
-      .find('[data-test="nav-list"]')
-      .children()
+      .find('[data-test="nav-list"] .ant-tabs-nav-list > .ant-tabs-tab')
       .as('row-level-tabs');
 
     cy.get('@row-level-tabs').last().click();
@@ -141,8 +149,7 @@ describe('Dashboard tabs', () => {
     handleException();
     cy.get('[data-test="dashboard-component-tabs"]')
       .first()
-      .find('[data-test="nav-list"]')
-      .children()
+      .find('[data-test="nav-list"] .ant-tabs-nav-list > .ant-tabs-tab')
       .as('top-level-tabs');
 
     cy.get('@top-level-tabs').last().click();

diff --git a/superset-frontend/spec/javascripts/dashboard/components/DashboardBuilder_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/DashboardBuilder_spec.jsx
@@ -179,7 +179,7 @@ describe('DashboardBuilder', () => {
     expect(wrapper.find(TabContainer).prop('activeKey')).toBe(0);
 
     wrapper
-      .find('.dashboard-component-tabs .nav-tabs a')
+      .find('.dashboard-component-tabs .ant-tabs .ant-tabs-tab')
       .at(1)
       .simulate('click');
 

diff --git a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tab_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tab_spec.jsx
@@ -22,10 +22,8 @@ import { styledMount as mount } from 'spec/helpers/theming';
 import sinon from 'sinon';
 
 import DashboardComponent from 'src/dashboard/containers/DashboardComponent';
-import DeleteComponentModal from 'src/dashboard/components/DeleteComponentModal';
 import DragDroppable from 'src/dashboard/components/dnd/DragDroppable';
 import EditableTitle from 'src/components/EditableTitle';
-import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu';
 import Tab, {
   RENDER_TAB,
   RENDER_TAB_CONTENT,
@@ -96,42 +94,6 @@ describe('Tabs', () => {
         'New title',
       );
     });
-
-    it('should render a WithPopoverMenu', () => {
-      const wrapper = setup();
-      expect(wrapper.find(WithPopoverMenu)).toExist();
-    });
-
-    it('should render a DeleteComponentModal when focused if its not the only tab', () => {
-      let wrapper = setup();
-      wrapper.find(WithPopoverMenu).simulate('click'); // focus
-      expect(wrapper.find(DeleteComponentModal)).not.toExist();
-
-      wrapper = setup({ editMode: true });
-      wrapper.find(WithPopoverMenu).simulate('click');
-      expect(wrapper.find(DeleteComponentModal)).toExist();
-
-      wrapper = setup({
-        editMode: true,
-        parentComponent: {
-          ...props.parentComponent,
-          children: props.parentComponent.children.slice(0, 1),
-        },
-      });
-      wrapper.find(WithPopoverMenu).simulate('click');
-      expect(wrapper.find(DeleteComponentModal)).not.toExist();
-    });
-
-    it('should show modal when clicked delete icon', () => {
-      const deleteComponent = sinon.spy();
-      const wrapper = setup({ editMode: true, deleteComponent });
-      wrapper.find(WithPopoverMenu).simulate('click'); // focus
-      wrapper.find('.icon-button').simulate('click');
-
-      const modal = document.getElementsByClassName('ant-modal');
-      expect(modal).toHaveLength(1);
-      expect(deleteComponent.callCount).toBe(0);
-    });
   });
 
   describe('renderType=RENDER_TAB_CONTENT', () => {

diff --git a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tabs_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tabs_spec.jsx
@@ -18,11 +18,12 @@
  */
 import { Provider } from 'react-redux';
 import React from 'react';
-import { mount, shallow } from 'enzyme';
+import { shallow } from 'enzyme';
 import sinon from 'sinon';
-import { Tabs as BootstrapTabs, Tab as BootstrapTab } from 'react-bootstrap';
-import { supersetTheme, ThemeProvider } from '@superset-ui/core';
+import { LineEditableTabs } from 'src/common/components/Tabs';
+import { Modal } from 'src/common/components';
 
+import { styledMount as mount } from 'spec/helpers/theming';
 import DashboardComponent from 'src/dashboard/containers/DashboardComponent';
 import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton';
 import HoverMenu from 'src/dashboard/components/menu/HoverMenu';
@@ -54,6 +55,7 @@ describe('Tabs', () => {
     deleteComponent() {},
     updateComponents() {},
     logEvent() {},
+    setMountedTab() {},
   };
 
   function setup(overrideProps) {
@@ -65,10 +67,6 @@ describe('Tabs', () => {
           <Tabs {...props} {...overrideProps} />
         </WithDragDropContext>
       </Provider>,
-      {
-        wrappingComponent: ThemeProvider,
-        wrappingComponentProps: { theme: supersetTheme },
-      },
     );
     return wrapper;
   }
@@ -79,31 +77,23 @@ describe('Tabs', () => {
     expect(wrapper.find(DragDroppable)).toExist();
   });
 
-  it('should render BootstrapTabs', () => {
+  it('should render non-editable tabs', () => {
     const wrapper = setup();
-    expect(wrapper.find(BootstrapTabs)).toExist();
+    expect(wrapper.find(LineEditableTabs)).toExist();
+    expect(wrapper.find('.ant-tabs-nav-add').exists()).toBeFalsy();
   });
 
-  it('should set animation=true, mountOnEnter=true, and unmounOnExit=false on BootstrapTabs for perf', () => {
+  it('should render a tab pane for each child', () => {
     const wrapper = setup();
-    const tabProps = wrapper.find(BootstrapTabs).props();
-    expect(tabProps.animation).toBe(true);
-    expect(tabProps.mountOnEnter).toBe(true);
-    expect(tabProps.unmountOnExit).toBe(false);
-  });
-
-  it('should render a BootstrapTab for each child', () => {
-    const wrapper = setup();
-    expect(wrapper.find(BootstrapTab)).toHaveLength(
+    expect(wrapper.find(LineEditableTabs.TabPane)).toHaveLength(
       props.component.children.length,
     );
   });
 
-  it('should render an extra (+) BootstrapTab in editMode', () => {
+  it('should render editable tabs in editMode', () => {
     const wrapper = setup({ editMode: true });
-    expect(wrapper.find(BootstrapTab)).toHaveLength(
-      props.component.children.length + 1,
-    );
+    expect(wrapper.find(LineEditableTabs)).toExist();
+    expect(wrapper.find('.ant-tabs-nav-add')).toExist();
   });
 
   it('should render a DashboardComponent for each child', () => {
@@ -118,7 +108,7 @@ describe('Tabs', () => {
     const createComponent = sinon.spy();
     const wrapper = setup({ editMode: true, createComponent });
     wrapper
-      .find('.dashboard-component-tabs .nav-tabs a')
+      .find('[data-test="dashboard-component-tabs"] .ant-tabs-nav-add')
       .last()
       .simulate('click');
 
@@ -129,7 +119,7 @@ describe('Tabs', () => {
     const onChangeTab = sinon.spy();
     const wrapper = setup({ editMode: true, onChangeTab });
     wrapper
-      .find('.dashboard-component-tabs .nav-tabs a')
+      .find('[data-test="dashboard-component-tabs"] .ant-tabs-tab')
       .at(1) // will not call if it is already selected
       .simulate('click');
 
@@ -140,7 +130,9 @@ describe('Tabs', () => {
     const onChangeTab = sinon.spy();
     const wrapper = setup({ editMode: true, onChangeTab });
     wrapper
-      .find('.dashboard-component-tabs .nav-tabs a .short-link-trigger')
+      .find(
+        '[data-test="dashboard-component-tabs"] .ant-tabs-tab [data-test="short-link-button"]',
+      )
       .at(1) // will not call if it is already selected
       .simulate('click');
 
@@ -186,4 +178,13 @@ describe('Tabs', () => {
     wrapper = shallow(<Tabs {...directLinkProps} />);
     expect(wrapper.state('tabIndex')).toBe(1);
   });
+
+  it('should render Modal when clicked remove tab button', () => {
+    const deleteComponent = sinon.spy();
+    const modalMock = jest.spyOn(Modal, 'confirm');
+    const wrapper = setup({ editMode: true, deleteComponent });
+    wrapper.find('.ant-tabs-tab-remove').at(0).simulate('click');
+    expect(modalMock.mock.calls).toHaveLength(1);
+    expect(deleteComponent.callCount).toBe(0);
+  });
 });
diff --git a/superset-frontend/src/common/components/Tabs/Tabs.tsx b/superset-frontend/src/common/components/Tabs/Tabs.tsx
@@ -40,6 +40,24 @@ const StyledTabs = styled(AntdTabs, {
     &.ant-tabs-tab-active .ant-tabs-tab-btn {
       color: inherit;
     }
+
+    &:hover {
+      .anchor-link-container {
+        cursor: pointer;
+
+        .fa.fa-link {
+          visibility: visible;
+        }
+      }
+    }
+
+    .short-link-trigger.btn {
+      padding: 0 ${({ theme }) => theme.gridUnit}px;
+
+      & > .fa.fa-link {
+        top: 0;
+      }
+    }
   }
 
   ${({ fullWidth }) =>
@@ -124,15 +142,37 @@ EditableTabs.TabPane.defaultProps = {
   ),
 };
 
-const StyledCardTabs = styled(EditableTabs)``;
+const StyledLineEditableTabs = styled(EditableTabs)`
+  &.ant-tabs-card > .ant-tabs-nav .ant-tabs-tab {
+    margin: 0 ${({ theme }) => theme.gridUnit * 4}px;
+    padding: ${({ theme }) => `${theme.gridUnit * 3}px ${theme.gridUnit}px`};
+    background: transparent;
+    border: none;
+  }
+
+  &.ant-tabs-card > .ant-tabs-nav .ant-tabs-ink-bar {
+    visibility: visible;
+  }
+
+  .ant-tabs-tab-btn {
+    font-size: ${({ theme }) => theme.typography.sizes.m}px;
+  }
+
+  .ant-tabs-tab-remove {
+    margin-left: 0;
+    padding-right: 0;
+  }
 
-const CardTabs = Object.assign(StyledCardTabs, {
+  .ant-tabs-nav-add {
+    min-width: unset !important;
+    background: transparent !important;
+    border: none !important;
+  }
+`;
+
+const LineEditableTabs = Object.assign(StyledLineEditableTabs, {
   TabPane: StyledTabPane,
 });
 
-CardTabs.defaultProps = {
-  type: 'card',
-};
-
 export default Tabs;
-export { CardTabs, EditableTabs };
+export { EditableTabs, LineEditableTabs };
diff --git a/superset-frontend/src/components/URLShortLinkButton.jsx b/superset-frontend/src/components/URLShortLinkButton.jsx
@@ -48,7 +48,8 @@ class URLShortLinkButton extends React.Component {
     }));
   }
 
-  getCopyUrl() {
+  getCopyUrl(e) {
+    e.stopPropagation();
     getShortUrl(this.props.url)
       .then(this.onShortUrlSuccess)
       .catch(this.props.addDangerToast);

diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder.jsx b/superset-frontend/src/dashboard/components/DashboardBuilder.jsx
@@ -101,7 +101,7 @@ class DashboardBuilder extends React.Component {
   static shouldFocusTabs(event, container) {
     // don't focus the tabs when we click on a tab
     return (
-      event.target.tagName === 'UL' ||
+      event.target.className === 'ant-tabs-nav-wrap' ||
       (/icon-button/.test(event.target.className) &&
         container.contains(event.target))
     );