Skip to content

Commit

Permalink
Merge pull request apache#109 from graceguo-supercat/gg-CherryPickDas…
Browse files Browse the repository at this point in the history
…hboardImprovementPRs

[cherry-picks] a few dashboard improvement PRs
  • Loading branch information
Grace Guo committed Sep 13, 2018
2 parents 20a6db9 + d9d8635 commit 0ca12f8
Show file tree
Hide file tree
Showing 9 changed files with 145 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { expect } from 'chai';
import sinon from 'sinon';

import DashboardComponent from '../../../../../src/dashboard/containers/DashboardComponent';
import DeleteComponentButton from '../../../../../src/dashboard/components/DeleteComponentButton';
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';
Expand Down Expand Up @@ -86,14 +86,14 @@ describe('Tabs', () => {
expect(wrapper.find(WithPopoverMenu)).to.have.length(1);
});

it('should render a DeleteComponentButton when focused if its not the only tab', () => {
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(DeleteComponentButton)).to.have.length(0);
expect(wrapper.find(DeleteComponentModal)).to.have.length(0);

wrapper = setup({ editMode: true });
wrapper.find(WithPopoverMenu).simulate('click');
expect(wrapper.find(DeleteComponentButton)).to.have.length(1);
expect(wrapper.find(DeleteComponentModal)).to.have.length(1);

wrapper = setup({
editMode: true,
Expand All @@ -103,16 +103,18 @@ describe('Tabs', () => {
},
});
wrapper.find(WithPopoverMenu).simulate('click');
expect(wrapper.find(DeleteComponentButton)).to.have.length(0);
expect(wrapper.find(DeleteComponentModal)).to.have.length(0);
});

it('should call deleteComponent when deleted', () => {
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(DeleteComponentButton).simulate('click');
wrapper.find('.icon-button').simulate('click');

expect(deleteComponent.callCount).to.equal(1);
const modal = document.getElementsByClassName('modal');
expect(modal).to.have.length(1);
expect(deleteComponent.callCount).to.equal(0);
});
});

Expand Down
11 changes: 7 additions & 4 deletions superset/assets/src/components/ModalTrigger.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import Button from './Button';
const propTypes = {
animation: PropTypes.bool,
triggerNode: PropTypes.node.isRequired,
modalTitle: PropTypes.node.isRequired,
modalTitle: PropTypes.node,
modalBody: PropTypes.node, // not required because it can be generated by beforeOpen
modalFooter: PropTypes.node,
beforeOpen: PropTypes.func,
Expand All @@ -28,6 +28,7 @@ const defaultProps = {
isMenuItem: false,
bsSize: null,
className: '',
modalTitle: '',
};

export default class ModalTrigger extends React.Component {
Expand Down Expand Up @@ -59,9 +60,11 @@ export default class ModalTrigger extends React.Component {
bsSize={this.props.bsSize}
className={this.props.className}
>
<Modal.Header closeButton>
<Modal.Title>{this.props.modalTitle}</Modal.Title>
</Modal.Header>
{this.props.modalTitle &&
<Modal.Header closeButton>
<Modal.Title>{this.props.modalTitle}</Modal.Title>
</Modal.Header>
}
<Modal.Body>
{this.props.modalBody}
</Modal.Body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class BuilderComponentPane extends React.PureComponent {
>
<div className="component-layer slide-content">
<div className="dashboard-builder-sidepane-header">
<span>{t('Insert')}</span>
<span>{t('Insert components')}</span>
<i
className="fa fa-times trigger"
onClick={this.props.toggleBuilderPane}
Expand Down Expand Up @@ -101,7 +101,7 @@ class BuilderComponentPane extends React.PureComponent {
role="none"
>
<i className="fa fa-arrow-left trigger" />
<span>{t('All components')}</span>
<span>{t('Your charts and filters')}</span>
</div>
<SliceAdder
height={
Expand Down
62 changes: 62 additions & 0 deletions superset/assets/src/dashboard/components/DeleteComponentModal.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import React from 'react';
import PropTypes from 'prop-types';
import { Button } from 'react-bootstrap';

import ModalTrigger from '../../components/ModalTrigger';
import { t } from '../../locales';

const propTypes = {
triggerNode: PropTypes.node.isRequired,
onDelete: PropTypes.func.isRequired,
};

export default class DeleteComponentModal extends React.PureComponent {
constructor(props) {
super(props);

this.modal = null;
this.close = this.close.bind(this);
this.deleteTab = this.deleteTab.bind(this);
this.setModalRef = this.setModalRef.bind(this);
}

setModalRef(ref) {
this.modal = ref;
}

close() {
this.modal.close();
}

deleteTab() {
this.modal.close();
this.props.onDelete();
}

render() {
return (
<ModalTrigger
ref={this.setModalRef}
triggerNode={this.props.triggerNode}
modalBody={
<div className="delete-component-modal">
<h1>{t('Delete dashboard tab?')}</h1>
<div>
Deleting a tab will remove all content within it. You may still
reverse this action with the <b>undo</b> button (cmd + z) until
you save your changes.
</div>
<div className="delete-modal-actions-container">
<Button onClick={this.close}>{t('Cancel')}</Button>
<Button bsStyle="primary" onClick={this.deleteTab}>
{t('Delete')}
</Button>
</div>
</div>
}
/>
);
}
}

DeleteComponentModal.propTypes = propTypes;
10 changes: 8 additions & 2 deletions superset/assets/src/dashboard/components/gridComponents/Tab.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import PropTypes from 'prop-types';
import DashboardComponent from '../../containers/DashboardComponent';
import DragDroppable from '../dnd/DragDroppable';
import EditableTitle from '../../../components/EditableTitle';
import DeleteComponentButton from '../DeleteComponentButton';
import DeleteComponentModal from '../DeleteComponentModal';
import WithPopoverMenu from '../menu/WithPopoverMenu';
import { componentShape } from '../../util/propShapes';
import { DASHBOARD_ROOT_DEPTH } from '../../util/constants';
Expand Down Expand Up @@ -178,6 +178,11 @@ export default class Tab extends React.PureComponent {
renderTab() {
const { isFocused } = this.state;
const { component, parentComponent, index, depth, editMode } = this.props;
const deleteTabIcon = (
<div className="icon-button">
<span className="fa fa-trash" />
</div>
);

return (
<DragDroppable
Expand All @@ -201,7 +206,8 @@ export default class Tab extends React.PureComponent {
parentComponent.children.length <= 1
? []
: [
<DeleteComponentButton
<DeleteComponentModal
triggerNode={deleteTabIcon}
onDelete={this.handleDeleteComponent}
/>,
]
Expand Down
23 changes: 12 additions & 11 deletions superset/assets/src/dashboard/components/menu/WithPopoverMenu.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ const defaultProps = {
onPressDelete() {},
menuItems: [],
isFocused: false,
shouldFocus: (event, container) => container.contains(event.target),
shouldFocus: (event, container) =>
container && container.contains(event.target),
style: null,
};

Expand All @@ -36,19 +37,19 @@ class WithPopoverMenu extends React.PureComponent {

componentWillReceiveProps(nextProps) {
if (nextProps.editMode && nextProps.isFocused && !this.state.isFocused) {
document.addEventListener('click', this.handleClick, true);
document.addEventListener('drag', this.handleClick, true);
document.addEventListener('click', this.handleClick);
document.addEventListener('drag', this.handleClick);
this.setState({ isFocused: true });
} else if (this.state.isFocused && !nextProps.editMode) {
document.removeEventListener('click', this.handleClick, true);
document.removeEventListener('drag', this.handleClick, true);
document.removeEventListener('click', this.handleClick);
document.removeEventListener('drag', this.handleClick);
this.setState({ isFocused: false });
}
}

componentWillUnmount() {
document.removeEventListener('click', this.handleClick, true);
document.removeEventListener('drag', this.handleClick, true);
document.removeEventListener('click', this.handleClick);
document.removeEventListener('drag', this.handleClick);
}

setRef(ref) {
Expand All @@ -69,15 +70,15 @@ class WithPopoverMenu extends React.PureComponent {
if (!disableClick && shouldFocus && !this.state.isFocused) {
// if not focused, set focus and add a window event listener to capture outside clicks
// this enables us to not set a click listener for ever item on a dashboard
document.addEventListener('click', this.handleClick, true);
document.addEventListener('drag', this.handleClick, true);
document.addEventListener('click', this.handleClick);
document.addEventListener('drag', this.handleClick);
this.setState(() => ({ isFocused: true }));
if (onChangeFocus) {
onChangeFocus(true);
}
} else if (!shouldFocus && this.state.isFocused) {
document.removeEventListener('click', this.handleClick, true);
document.removeEventListener('drag', this.handleClick, true);
document.removeEventListener('click', this.handleClick);
document.removeEventListener('drag', this.handleClick);
this.setState(() => ({ isFocused: false }));
if (onChangeFocus) {
onChangeFocus(false);
Expand Down
11 changes: 11 additions & 0 deletions superset/assets/src/dashboard/stylesheets/components/markdown.less
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
.dashboard-markdown {
overflow: hidden;

h4, h5 {
font-weight: 300;
}
h5 {
color: @gray-heading;
}
h6 {
font-weight: 400;
font-size: 12px;
}

.dashboard-component-chart-holder {
overflow-y: auto;
overflow-x: hidden;
Expand Down
36 changes: 32 additions & 4 deletions superset/assets/src/dashboard/stylesheets/dashboard.less
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,38 @@ body {
}
}

.modal img.loading {
width: 50px;
margin: 0;
position: relative;
.modal {
img.loading {
width: 50px;
margin: 0;
position: relative;
}

.modal-body {
padding: 24px 24px 29px 24px;

div {
margin-top: 24px;

&:first-child {
margin-top: 0;
}
}
}

.delete-modal-actions-container {
.btn {
margin-right: 16px;
&:last-child {
margin-right: 0;
}

&.btn-primary {
background: @pink !important;
border-color: @pink !important;
}
}
}
}

.react-bs-container-body {
Expand Down
1 change: 1 addition & 0 deletions superset/assets/src/dashboard/stylesheets/variables.less
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
@gray: #879399;
@gray-light: #CFD8DC;
@gray-bg: #f5f5f5;
@gray-heading: #A3A3A3;
@menu-hover: #F2F3F5;

/* builder component pane */
Expand Down

0 comments on commit 0ca12f8

Please sign in to comment.