From 894ca3c09b4bd5a4405dec1ff98f161d71d88839 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Mon, 26 Oct 2020 19:20:10 +0100 Subject: [PATCH] refactor: Use Modals from Antd instead of react-bootstrap (#11330) * Refactor ModalTrigger to use antd modal * Refactor a few components * dynamic width * Fix unit tests * Use i18n for button text --- .../dashboard/components/CssEditor_spec.jsx | 9 +++-- .../components/RefreshIntervalModal_spec.jsx | 15 ++++++-- .../components/gridComponents/Tab_spec.jsx | 2 +- .../components/DisplayQueryButton_spec.jsx | 8 ++++- .../sqllab/HighlightedSql_spec.jsx | 7 ++++ .../javascripts/sqllab/TableElement_spec.jsx | 13 +++++++ .../src/SqlLab/components/QueryTable.jsx | 2 +- superset-frontend/src/SqlLab/main.less | 6 ++-- .../src/common/components/Modal.tsx | 27 +++++++++++--- .../src/components/AlteredSliceTag.jsx | 3 +- .../src/components/ModalTrigger.jsx | 36 ++++++++----------- .../explore/components/DisplayQueryButton.jsx | 17 +++------ .../components/controls/TextAreaControl.jsx | 2 +- 13 files changed, 94 insertions(+), 53 deletions(-) diff --git a/superset-frontend/spec/javascripts/dashboard/components/CssEditor_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/CssEditor_spec.jsx index d631ab58d72..62460d04e5d 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/CssEditor_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/CssEditor_spec.jsx @@ -18,7 +18,7 @@ */ import React from 'react'; import { mount } from 'enzyme'; - +import { supersetTheme, ThemeProvider } from '@superset-ui/core'; import CssEditor from 'src/dashboard/components/CssEditor'; describe('CssEditor', () => { @@ -29,7 +29,12 @@ describe('CssEditor', () => { expect(React.isValidElement()).toBe(true); }); it('renders the trigger node', () => { - const wrapper = mount(); + const wrapper = mount(, { + wrappingComponent: ThemeProvider, + wrappingComponentProps: { + theme: supersetTheme, + }, + }); expect(wrapper.find('.fa-edit')).toExist(); }); }); diff --git a/superset-frontend/spec/javascripts/dashboard/components/RefreshIntervalModal_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/RefreshIntervalModal_spec.jsx index cde2864eaf3..e6210260d1d 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/RefreshIntervalModal_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/RefreshIntervalModal_spec.jsx @@ -22,6 +22,15 @@ import { mount, shallow } from 'enzyme'; import ModalTrigger from 'src/components/ModalTrigger'; import RefreshIntervalModal from 'src/dashboard/components/RefreshIntervalModal'; import { Alert } from 'react-bootstrap'; +import { supersetTheme, ThemeProvider } from '@superset-ui/core'; + +const getMountWrapper = props => + mount(, { + wrappingComponent: ThemeProvider, + wrappingComponentProps: { + theme: supersetTheme, + }, + }); describe('RefreshIntervalModal', () => { const mockedProps = { @@ -36,15 +45,15 @@ describe('RefreshIntervalModal', () => { ).toBe(true); }); it('renders the trigger node', () => { - const wrapper = mount(); + const wrapper = getMountWrapper(mockedProps); expect(wrapper.find('.fa-edit')).toExist(); }); it('should render a interval seconds', () => { - const wrapper = mount(); + const wrapper = getMountWrapper(mockedProps); expect(wrapper.prop('refreshFrequency')).toEqual(10); }); it('should change refreshFrequency with edit mode', () => { - const wrapper = mount(); + const wrapper = getMountWrapper(mockedProps); wrapper.instance().handleFrequencyChange({ value: 30 }); wrapper.instance().onSave(); expect(mockedProps.onChange).toHaveBeenCalled(); diff --git a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tab_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tab_spec.jsx index 059de56689d..f6c0a67d8f0 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tab_spec.jsx +++ b/superset-frontend/spec/javascripts/dashboard/components/gridComponents/Tab_spec.jsx @@ -126,7 +126,7 @@ describe('Tabs', () => { wrapper.find(WithPopoverMenu).simulate('click'); // focus wrapper.find('.icon-button').simulate('click'); - const modal = document.getElementsByClassName('modal'); + const modal = document.getElementsByClassName('ant-modal'); expect(modal).toHaveLength(1); expect(deleteComponent.callCount).toBe(0); }); diff --git a/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx b/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx index 4e75cda0b9f..e49487ebd87 100644 --- a/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx +++ b/superset-frontend/spec/javascripts/explore/components/DisplayQueryButton_spec.jsx @@ -21,6 +21,7 @@ import { mount } from 'enzyme'; import ModalTrigger from 'src/components/ModalTrigger'; import { DisplayQueryButton } from 'src/explore/components/DisplayQueryButton'; import { MenuItem } from 'react-bootstrap'; +import { supersetTheme, ThemeProvider } from '@superset-ui/core'; describe('DisplayQueryButton', () => { const defaultProps = { @@ -43,7 +44,12 @@ describe('DisplayQueryButton', () => { ); }); it('renders a dropdown', () => { - const wrapper = mount(); + const wrapper = mount(, { + wrappingComponent: ThemeProvider, + wrappingComponentProps: { + theme: supersetTheme, + }, + }); expect(wrapper.find(ModalTrigger)).toHaveLength(3); expect(wrapper.find(MenuItem)).toHaveLength(5); }); diff --git a/superset-frontend/spec/javascripts/sqllab/HighlightedSql_spec.jsx b/superset-frontend/spec/javascripts/sqllab/HighlightedSql_spec.jsx index 80f550c9787..66ddf287d2c 100644 --- a/superset-frontend/spec/javascripts/sqllab/HighlightedSql_spec.jsx +++ b/superset-frontend/spec/javascripts/sqllab/HighlightedSql_spec.jsx @@ -22,6 +22,7 @@ import { mount, shallow } from 'enzyme'; import HighlightedSql from 'src/SqlLab/components/HighlightedSql'; import ModalTrigger from 'src/components/ModalTrigger'; +import { supersetTheme, ThemeProvider } from '@superset-ui/core'; describe('HighlightedSql', () => { const sql = @@ -45,6 +46,12 @@ describe('HighlightedSql', () => { shrink maxWidth={5} />, + { + wrappingComponent: ThemeProvider, + wrappingComponentProps: { + theme: supersetTheme, + }, + }, ); const pre = wrapper.find('pre'); expect(pre).toHaveLength(1); diff --git a/superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx b/superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx index a0f242bb1c0..6c3b8040edc 100644 --- a/superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx +++ b/superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx @@ -20,6 +20,7 @@ import React from 'react'; import { mount, shallow } from 'enzyme'; import { Provider } from 'react-redux'; import configureStore from 'redux-mock-store'; +import { supersetTheme, ThemeProvider } from '@superset-ui/core'; import Link from 'src/components/Link'; import TableElement from 'src/SqlLab/components/TableElement'; @@ -54,6 +55,12 @@ describe('TableElement', () => { , + { + wrappingComponent: ThemeProvider, + wrappingComponentProps: { + theme: supersetTheme, + }, + }, ); }); it('sorts columns', () => { @@ -71,6 +78,12 @@ describe('TableElement', () => { , + { + wrappingComponent: ThemeProvider, + wrappingComponentProps: { + theme: supersetTheme, + }, + }, ); expect(mockedActions.collapseTable.called).toBe(false); wrapper.find('.table-name').simulate('click'); diff --git a/superset-frontend/src/SqlLab/components/QueryTable.jsx b/superset-frontend/src/SqlLab/components/QueryTable.jsx index 444952ecf3d..cd29176d47d 100644 --- a/superset-frontend/src/SqlLab/components/QueryTable.jsx +++ b/superset-frontend/src/SqlLab/components/QueryTable.jsx @@ -142,7 +142,6 @@ const QueryTable = props => { if (q.resultsKey) { q.output = ( @@ -161,6 +160,7 @@ const QueryTable = props => { displayLimit={props.displayLimit} /> } + responsive /> ); } else { diff --git a/superset-frontend/src/SqlLab/main.less b/superset-frontend/src/SqlLab/main.less index 14bef7df2af..58aae6a1e9b 100644 --- a/superset-frontend/src/SqlLab/main.less +++ b/superset-frontend/src/SqlLab/main.less @@ -466,11 +466,11 @@ div.tablePopover { display: inline-block; } -.ResultsModal .modal-body { - min-height: 600px; +.ResultsModal .ant-modal-body { + min-height: 560px; } -.modal-body { +.ant-modal-body { overflow: auto; } diff --git a/superset-frontend/src/common/components/Modal.tsx b/superset-frontend/src/common/components/Modal.tsx index 55f9f47eea5..7e7606ded56 100644 --- a/superset-frontend/src/common/components/Modal.tsx +++ b/superset-frontend/src/common/components/Modal.tsx @@ -18,9 +18,10 @@ */ import React from 'react'; import { isNil } from 'lodash'; -import { styled, t } from '@superset-ui/core'; +import { styled, SupersetThemeProps, t } from '@superset-ui/core'; import { Modal as BaseModal } from 'src/common/components'; import Button from 'src/components/Button'; +import { css } from '@emotion/core'; interface ModalProps { className?: string; @@ -33,12 +34,25 @@ interface ModalProps { show: boolean; title: React.ReactNode; width?: string; + maxWidth?: string; + responsive?: boolean; hideFooter?: boolean; centered?: boolean; footer?: React.ReactNode; } -const StyledModal = styled(BaseModal)` +interface StyledModalProps extends SupersetThemeProps { + maxWidth?: string; + responsive?: boolean; +} + +const StyledModal = styled(BaseModal)` + ${({ responsive, maxWidth }) => + responsive && + css` + max-width: ${maxWidth ?? '900px'}; + `} + .ant-modal-header { background-color: ${({ theme }) => theme.colors.grayscale.light4}; border-radius: ${({ theme }) => theme.borderRadius}px @@ -94,11 +108,13 @@ export default function Modal({ disablePrimaryButton = false, onHide, onHandledPrimaryAction, - primaryButtonName, + primaryButtonName = t('OK'), primaryButtonType = 'primary', show, title, width, + maxWidth, + responsive = false, centered, footer, hideFooter, @@ -121,12 +137,15 @@ export default function Modal({ ] : footer; + const modalWidth = width || responsive ? 'calc(100vw - 24px)' : '600px'; return ( ); } diff --git a/superset-frontend/src/components/ModalTrigger.jsx b/superset-frontend/src/components/ModalTrigger.jsx index 83b15f4e754..90c8210d850 100644 --- a/superset-frontend/src/components/ModalTrigger.jsx +++ b/superset-frontend/src/components/ModalTrigger.jsx @@ -18,13 +18,12 @@ */ import React from 'react'; import PropTypes from 'prop-types'; -import { Modal, MenuItem } from 'react-bootstrap'; - +import { MenuItem } from 'react-bootstrap'; +import Modal from 'src/common/components/Modal'; import Button from 'src/components/Button'; const propTypes = { dialogClassName: PropTypes.string, - animation: PropTypes.bool, triggerNode: PropTypes.node.isRequired, modalTitle: PropTypes.node, modalBody: PropTypes.node, // not required because it can be generated by beforeOpen @@ -33,19 +32,18 @@ const propTypes = { onExit: PropTypes.func, isButton: PropTypes.bool, isMenuItem: PropTypes.bool, - bsSize: PropTypes.oneOf(['large', 'small']), // react-bootstrap also supports 'sm', 'lg' but we're keeping it simple. className: PropTypes.string, tooltip: PropTypes.string, - backdrop: PropTypes.oneOf(['static', true, false]), + width: PropTypes.string, + maxWidth: PropTypes.string, + responsive: PropTypes.bool, }; const defaultProps = { - animation: true, beforeOpen: () => {}, onExit: () => {}, isButton: false, isMenuItem: false, - bsSize: null, className: '', modalTitle: '', }; @@ -73,24 +71,18 @@ export default class ModalTrigger extends React.Component { renderModal() { return ( - {this.props.modalTitle && ( - - {this.props.modalTitle} - - )} - {this.props.modalBody} - {this.props.modalFooter && ( - {this.props.modalFooter} - )} + {this.props.modalBody} ); } diff --git a/superset-frontend/src/explore/components/DisplayQueryButton.jsx b/superset-frontend/src/explore/components/DisplayQueryButton.jsx index b098b401a15..d97c8e6ff3f 100644 --- a/superset-frontend/src/explore/components/DisplayQueryButton.jsx +++ b/superset-frontend/src/explore/components/DisplayQueryButton.jsx @@ -58,16 +58,12 @@ SyntaxHighlighter.registerLanguage('json', jsonSyntax); const propTypes = { onOpenInEditor: PropTypes.func, - animation: PropTypes.bool, queryResponse: PropTypes.object, chartStatus: PropTypes.string, chartHeight: PropTypes.string.isRequired, latestQueryFormData: PropTypes.object.isRequired, slice: PropTypes.object, }; -const defaultProps = { - animation: true, -}; export class DisplayQueryButton extends React.PureComponent { constructor(props) { @@ -234,7 +230,7 @@ export class DisplayQueryButton extends React.PureComponent { } render() { - const { animation, chartHeight, slice } = this.props; + const { chartHeight, slice } = this.props; return ( )} {t('View query')} } modalTitle={t('View query')} - bsSize="large" beforeOpen={() => this.beforeOpen('query')} modalBody={this.renderQueryModalBody()} + responsive /> {t('View results')}} modalTitle={t('View results')} - bsSize="large" beforeOpen={() => this.beforeOpen('results')} modalBody={this.renderResultsModalBody()} + responsive /> {t('View samples')}} modalTitle={t('View samples')} - bsSize="large" beforeOpen={() => this.beforeOpen('samples')} modalBody={this.renderSamplesModalBody()} + responsive /> {this.state.sqlSupported && ( @@ -315,7 +307,6 @@ export class DisplayQueryButton extends React.PureComponent { } DisplayQueryButton.propTypes = propTypes; -DisplayQueryButton.defaultProps = defaultProps; function mapDispatchToProps(dispatch) { return bindActionCreators({ sliceUpdated }, dispatch); diff --git a/superset-frontend/src/explore/components/controls/TextAreaControl.jsx b/superset-frontend/src/explore/components/controls/TextAreaControl.jsx index d0c33ec4ef2..b1bb5d3ca7f 100644 --- a/superset-frontend/src/explore/components/controls/TextAreaControl.jsx +++ b/superset-frontend/src/explore/components/controls/TextAreaControl.jsx @@ -124,7 +124,6 @@ export default class TextAreaControl extends React.Component { {this.renderEditor()} {this.props.offerEditInModal && ( @@ -133,6 +132,7 @@ export default class TextAreaControl extends React.Component { } modalBody={this.renderModalBody(true)} + responsive /> )}