-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Edit Dashboard title and Slice title in place #2940
Changes from 5 commits
e6d3828
52bc767
0c7bcb8
6aadc17
74315dc
5691f32
497485c
280a55c
59e3027
cfbbc01
6d3cc34
1e613de
1422f26
d52273e
2a42cee
98d4418
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import TooltipWrapper from './TooltipWrapper'; | ||
|
||
class EditableTitle extends React.PureComponent { | ||
constructor(props) { | ||
super(props); | ||
this.state = { | ||
isEditing: false, | ||
title: this.props.title, | ||
lastTitle: this.props.title, | ||
}; | ||
this.handleClick = this.handleClick.bind(this); | ||
this.handleBlur = this.handleBlur.bind(this); | ||
this.handleChange = this.handleChange.bind(this); | ||
} | ||
handleClick() { | ||
if (!this.props.canEdit) { | ||
return; | ||
} | ||
|
||
this.setState({ | ||
isEditing: true, | ||
}); | ||
} | ||
handleBlur() { | ||
if (!this.props.canEdit) { | ||
return; | ||
} | ||
|
||
this.setState({ | ||
isEditing: false, | ||
}); | ||
|
||
if (this.state.lastTitle !== this.state.title) { | ||
this.setState({ | ||
lastTitle: this.state.title, | ||
}); | ||
this.props.onSaveTitle(this.state.title); | ||
} | ||
} | ||
handleChange(ev) { | ||
if (!this.props.canEdit) { | ||
return; | ||
} | ||
|
||
this.setState({ | ||
title: ev.target.value, | ||
}); | ||
} | ||
render() { | ||
return ( | ||
<span className="editable-title"> | ||
<TooltipWrapper | ||
label="title" | ||
tooltip={this.props.canEdit ? 'click to edit title' : 'You don\'t have the rights to alter this title.'} | ||
> | ||
<input | ||
required | ||
type={this.state.isEditing ? 'text' : 'button'} | ||
value={this.state.title} | ||
onChange={this.handleChange} | ||
onBlur={this.handleBlur} | ||
onClick={this.handleClick} | ||
/> | ||
</TooltipWrapper> | ||
</span> | ||
); | ||
} | ||
} | ||
EditableTitle.propTypes = { | ||
title: PropTypes.string, | ||
canEdit: PropTypes.bool, | ||
onSaveTitle: PropTypes.func.isRequired, | ||
}; | ||
EditableTitle.defaultProps = { | ||
title: 'Title', | ||
canEdit: false, | ||
}; | ||
|
||
export default EditableTitle; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,11 +7,13 @@ import { Alert, Collapse, Panel } from 'react-bootstrap'; | |
import visMap from '../../../visualizations/main'; | ||
import { d3format } from '../../modules/utils'; | ||
import ExploreActionButtons from './ExploreActionButtons'; | ||
import EditableTitle from '../../components/EditableTitle'; | ||
import FaveStar from '../../components/FaveStar'; | ||
import TooltipWrapper from '../../components/TooltipWrapper'; | ||
import Timer from '../../components/Timer'; | ||
import { getExploreUrl } from '../exploreUtils'; | ||
import { getFormDataFromControls } from '../stores/store'; | ||
import { serialize } from '../../../utils/common'; | ||
import CachedLabel from '../../components/CachedLabel'; | ||
|
||
const CHART_STATUS_MAP = { | ||
|
@@ -23,6 +25,7 @@ const CHART_STATUS_MAP = { | |
const propTypes = { | ||
actions: PropTypes.object.isRequired, | ||
alert: PropTypes.string, | ||
can_overwrite: PropTypes.bool.isRequired, | ||
can_download: PropTypes.bool.isRequired, | ||
chartStatus: PropTypes.string, | ||
chartUpdateEndTime: PropTypes.number, | ||
|
@@ -39,6 +42,8 @@ const propTypes = { | |
queryResponse: PropTypes.object, | ||
triggerRender: PropTypes.bool, | ||
standalone: PropTypes.bool, | ||
datasourceType: PropTypes.string, | ||
datasourceId: PropTypes.number, | ||
}; | ||
|
||
class ChartContainer extends React.PureComponent { | ||
|
@@ -145,6 +150,19 @@ class ChartContainer extends React.PureComponent { | |
this.props.actions.runQuery(this.props.formData, true); | ||
} | ||
|
||
updateChartTitle(newTitle) { | ||
const params = {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this is a bit more elegant:
|
||
params.slice_name = newTitle; | ||
params.action = 'overwrite'; | ||
params.form_data = this.props.formData; | ||
const saveUrl = '/superset/explore/' + | ||
`${this.props.datasourceType}/${this.props.datasourceId}/?${serialize(params)}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can reuse the getExploreUrl method here? https://github.com/airbnb/superset/blob/master/superset/assets/javascripts/explore/exploreUtils.js#L4 Or maybe it's time soon to remove move save/overwrite out of |
||
this.props.actions.saveSlice(saveUrl) | ||
.then(() => { | ||
this.props.actions.updateChartTitle(newTitle); | ||
}); | ||
} | ||
|
||
renderChartTitle() { | ||
let title; | ||
if (this.props.slice) { | ||
|
@@ -240,7 +258,11 @@ class ChartContainer extends React.PureComponent { | |
id="slice-header" | ||
className="clearfix panel-title-large" | ||
> | ||
{this.renderChartTitle()} | ||
<EditableTitle | ||
title={this.renderChartTitle()} | ||
canEdit={this.props.can_overwrite} | ||
onSaveTitle={this.updateChartTitle.bind(this)} | ||
/> | ||
|
||
{this.props.slice && | ||
<span> | ||
|
@@ -304,6 +326,7 @@ function mapStateToProps(state) { | |
const formData = getFormDataFromControls(state.controls); | ||
return { | ||
alert: state.chartAlert, | ||
can_overwrite: state.can_overwrite, | ||
can_download: state.can_download, | ||
chartStatus: state.chartStatus, | ||
chartUpdateEndTime: state.chartUpdateEndTime, | ||
|
@@ -320,7 +343,8 @@ function mapStateToProps(state) { | |
table_name: formData.datasource_name, | ||
viz_type: formData.viz_type, | ||
triggerRender: state.triggerRender, | ||
datasourceType: state.datasource ? state.datasource.type : null, | ||
datasourceType: state.datasource_type, | ||
datasourceId: state.datasource_id, | ||
}; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import React from 'react'; | ||
import { shallow } from 'enzyme'; | ||
import { describe, it } from 'mocha'; | ||
import sinon from 'sinon'; | ||
import { expect } from 'chai'; | ||
|
||
import EditableTable from '../../../javascripts/components/EditableTitle'; | ||
|
||
describe('EditableTitle', () => { | ||
const callback = sinon.spy(); | ||
const mockProps = { | ||
title: 'my title', | ||
canEdit: true, | ||
onSaveTitle: callback, | ||
}; | ||
const mockEvent = { | ||
target: { | ||
value: 'new title', | ||
}, | ||
}; | ||
const editableWrapper = shallow(<EditableTable {...mockProps} />); | ||
const notEditableWrapper = shallow(<EditableTable title="my title" />); | ||
it('is valid', () => { | ||
expect( | ||
React.isValidElement(<EditableTable {...mockProps} />), | ||
).to.equal(true); | ||
}); | ||
it('should render title', () => { | ||
const titleElement = editableWrapper.find('input'); | ||
expect(titleElement.props().value).to.equal('my title'); | ||
expect(titleElement.props().type).to.equal('button'); | ||
}); | ||
|
||
describe('should handle click', () => { | ||
it('should change title', () => { | ||
editableWrapper.find('input').simulate('click'); | ||
expect(editableWrapper.find('input').props().type).to.equal('text'); | ||
}); | ||
it('should not change title', () => { | ||
notEditableWrapper.find('input').simulate('click'); | ||
expect(notEditableWrapper.find('input').props().type).to.equal('button'); | ||
}); | ||
}); | ||
|
||
describe('should handle change', () => { | ||
afterEach(() => { | ||
editableWrapper.setState({ title: 'my title' }); | ||
editableWrapper.setState({ lastTitle: 'my title' }); | ||
}); | ||
it('should change title', () => { | ||
editableWrapper.find('input').simulate('change', mockEvent); | ||
expect(editableWrapper.find('input').props().value).to.equal('new title'); | ||
}); | ||
it('should not change title', () => { | ||
notEditableWrapper.find('input').simulate('change', mockEvent); | ||
expect(editableWrapper.find('input').props().value).to.equal('my title'); | ||
}); | ||
}); | ||
|
||
describe('should handle blur', () => { | ||
beforeEach(() => { | ||
editableWrapper.find('input').simulate('click'); | ||
expect(editableWrapper.find('input').props().type).to.equal('text'); | ||
}); | ||
afterEach(() => { | ||
callback.reset(); | ||
editableWrapper.setState({ title: 'my title' }); | ||
editableWrapper.setState({ lastTitle: 'my title' }); | ||
}); | ||
|
||
it('should trigger callback', () => { | ||
editableWrapper.setState({ title: 'new title' }); | ||
editableWrapper.find('input').simulate('blur'); | ||
expect(editableWrapper.find('input').props().type).to.equal('button'); | ||
expect(callback.callCount).to.equal(1); | ||
expect(callback.getCall(0).args[0]).to.equal('new title'); | ||
}); | ||
it('should not trigger callback', () => { | ||
editableWrapper.find('input').simulate('blur'); | ||
expect(editableWrapper.find('input').props().type).to.equal('button'); | ||
// no change | ||
expect(callback.callCount).to.equal(0); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big deal but we've been setting
propTypes
anddefaultProps
at the top of the file in most components.