You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2020/11/28 08:59:45 UTC
[incubator-superset] branch master updated: fix: Adding and
removing annotations (#11811)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new 43c69d5 fix: Adding and removing annotations (#11811)
43c69d5 is described below
commit 43c69d52a4bc0ccfa8457d17b4c469f30d2b9b43
Author: Agata Stawarz <47...@users.noreply.github.com>
AuthorDate: Sat Nov 28 09:59:16 2020 +0100
fix: Adding and removing annotations (#11811)
* Fix creating new annotation layer and refactor AnnotationLayer component
* Refactor checking overrides keys in AnnotationLayer
* Fix adding and editing annotations. Refactor AnnotationLayer submit and remove functions
* Remove unused parent prop from AnnotationLayer
* Fix toggling annotation layer popover
* Revert extracting annotation props to nested object
* Clean up AnnotationLayer component
* Remove unnecessary import
* Use original name for identifying annotations in add/remove actions
* Change props order and rename removeAnnotation function
* Compare annotations by reference instead of name
---
.../components/controls/AnnotationLayer.jsx | 87 ++++++++++++----------
.../components/controls/AnnotationLayerControl.jsx | 53 +++++++------
2 files changed, 78 insertions(+), 62 deletions(-)
diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayer.jsx b/superset-frontend/src/explore/components/controls/AnnotationLayer.jsx
index b0f0569..3388fea 100644
--- a/superset-frontend/src/explore/components/controls/AnnotationLayer.jsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayer.jsx
@@ -120,8 +120,8 @@ export default class AnnotationLayer extends React.PureComponent {
intervalEndColumn,
} = props;
- const overridesKeys = Object.keys(overrides);
- if (overridesKeys.includes('since') || overridesKeys.includes('until')) {
+ // Only allow override whole time_range
+ if ('since' in overrides || 'until' in overrides) {
overrides.time_range = null;
delete overrides.since;
delete overrides.until;
@@ -130,7 +130,6 @@ export default class AnnotationLayer extends React.PureComponent {
this.state = {
// base
name,
- oldName: !this.props.name ? null : name,
annotationType,
sourceType,
value,
@@ -149,10 +148,9 @@ export default class AnnotationLayer extends React.PureComponent {
showMarkers,
hideLine,
// refData
- isNew: !this.props.name,
+ isNew: !name,
isLoadingOptions: true,
valueOptions: [],
- validationErrors: {},
};
this.submitAnnotation = this.submitAnnotation.bind(this);
this.deleteAnnotation = this.deleteAnnotation.bind(this);
@@ -237,7 +235,6 @@ export default class AnnotationLayer extends React.PureComponent {
this.setState({
annotationType,
sourceType: null,
- validationErrors: {},
value: null,
});
}
@@ -246,12 +243,7 @@ export default class AnnotationLayer extends React.PureComponent {
const { sourceType: prevSourceType } = this.state;
if (prevSourceType !== sourceType) {
- this.setState({
- sourceType,
- isLoadingOptions: true,
- validationErrors: {},
- value: null,
- });
+ this.setState({ sourceType, value: null, isLoadingOptions: true });
}
}
@@ -267,7 +259,7 @@ export default class AnnotationLayer extends React.PureComponent {
}
fetchOptions(annotationType, sourceType, isLoadingOptions) {
- if (isLoadingOptions === true) {
+ if (isLoadingOptions) {
if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) {
SupersetClient.get({
endpoint: '/annotationlayermodelview/api/read?',
@@ -310,28 +302,43 @@ export default class AnnotationLayer extends React.PureComponent {
}
deleteAnnotation() {
+ this.props.removeAnnotationLayer();
this.props.close();
- if (!this.state.isNew) {
- this.props.removeAnnotationLayer(this.state);
- }
}
applyAnnotation() {
- if (this.state.name.length) {
- const annotation = {};
- Object.keys(this.state).forEach(k => {
- if (this.state[k] !== null) {
- annotation[k] = this.state[k];
+ if (this.isValidForm()) {
+ const annotationFields = [
+ 'name',
+ 'annotationType',
+ 'sourceType',
+ 'color',
+ 'opacity',
+ 'style',
+ 'width',
+ 'showMarkers',
+ 'hideLine',
+ 'value',
+ 'overrides',
+ 'show',
+ 'titleColumn',
+ 'descriptionColumns',
+ 'timeColumn',
+ 'intervalEndColumn',
+ ];
+ const newAnnotation = {};
+ annotationFields.forEach(field => {
+ if (this.state[field] !== null) {
+ newAnnotation[field] = this.state[field];
}
});
- delete annotation.isNew;
- delete annotation.valueOptions;
- delete annotation.isLoadingOptions;
- delete annotation.validationErrors;
- annotation.color =
- annotation.color === AUTOMATIC_COLOR ? null : annotation.color;
- this.props.addAnnotationLayer(annotation);
- this.setState(prevState => ({ isNew: false, oldName: prevState.name }));
+
+ if (newAnnotation.color === AUTOMATIC_COLOR) {
+ newAnnotation.color = null;
+ }
+
+ this.props.addAnnotationLayer(newAnnotation);
+ this.setState({ isNew: false });
}
}
@@ -363,7 +370,7 @@ export default class AnnotationLayer extends React.PureComponent {
label = 'Annotation Layer';
description = 'Select the Annotation Layer you would like to use.';
} else {
- label = label = t('Chart');
+ label = t('Chart');
description = `Use a pre defined Superset Chart as a source for annotations and overlays.
your chart must be one of these visualization types:
[${this.getSupportedSourceTypes(annotationType)
@@ -502,7 +509,7 @@ export default class AnnotationLayer extends React.PureComponent {
label="Override time range"
description={`This controls whether the "time_range" field from the current
view should be passed down to the chart containing the annotation data.`}
- value={!!Object.keys(overrides).find(x => x === 'time_range')}
+ value={'time_range' in overrides}
onChange={v => {
delete overrides.time_range;
if (v) {
@@ -520,9 +527,7 @@ export default class AnnotationLayer extends React.PureComponent {
label="Override time grain"
description={`This controls whether the time grain field from the current
view should be passed down to the chart containing the annotation data.`}
- value={
- !!Object.keys(overrides).find(x => x === 'time_grain_sqla')
- }
+ value={'time_grain_sqla' in overrides}
onChange={v => {
delete overrides.time_grain_sqla;
delete overrides.granularity;
@@ -710,7 +715,7 @@ export default class AnnotationLayer extends React.PureComponent {
value={annotationType}
onChange={this.handleAnnotationType}
/>
- {!!supportedSourceTypes.length && (
+ {supportedSourceTypes.length > 0 && (
<SelectControl
hovered
description="Choose the source of your annotations"
@@ -728,9 +733,15 @@ export default class AnnotationLayer extends React.PureComponent {
{this.renderDisplayConfiguration()}
</div>
<div style={{ display: 'flex', justifyContent: 'space-between' }}>
- <Button buttonSize="sm" onClick={this.deleteAnnotation}>
- {!isNew ? t('Remove') : t('Cancel')}
- </Button>
+ {isNew ? (
+ <Button buttonSize="sm" onClick={() => this.props.close()}>
+ {t('Cancel')}
+ </Button>
+ ) : (
+ <Button buttonSize="sm" onClick={this.deleteAnnotation}>
+ {t('Remove')}
+ </Button>
+ )}
<div>
<Button
buttonSize="sm"
diff --git a/superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx b/superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx
index 5cdd535..48d3e14 100644
--- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx
@@ -58,7 +58,7 @@ const defaultProps = {
class AnnotationLayerControl extends React.PureComponent {
constructor(props) {
super(props);
- this.state = { popoverVisible: false };
+ this.state = { popoverVisible: {}, addedAnnotationIndex: null };
this.addAnnotationLayer = this.addAnnotationLayer.bind(this);
this.removeAnnotationLayer = this.removeAnnotationLayer.bind(this);
this.handleVisibleChange = this.handleVisibleChange.bind(this);
@@ -83,48 +83,50 @@ class AnnotationLayerControl extends React.PureComponent {
}
}
- addAnnotationLayer(annotationLayer) {
- const annotation = annotationLayer;
- let annotations = this.props.value.slice();
- const i = annotations.findIndex(
- x => x.name === (annotation.oldName || annotation.name),
- );
- delete annotation.oldName;
- if (i > -1) {
- annotations[i] = annotation;
+ addAnnotationLayer(originalAnnotation, newAnnotation) {
+ let annotations = this.props.value;
+ if (annotations.includes(originalAnnotation)) {
+ annotations = annotations.map(anno =>
+ anno === originalAnnotation ? newAnnotation : anno,
+ );
} else {
- annotations = annotations.concat(annotation);
+ annotations = [...annotations, newAnnotation];
+ this.setState({ addedAnnotationIndex: annotations.length - 1 });
}
- this.props.refreshAnnotationData(annotation);
+
+ this.props.refreshAnnotationData(newAnnotation);
this.props.onChange(annotations);
}
handleVisibleChange(visible, popoverKey) {
this.setState(prevState => ({
- popoverVisible: { ...prevState, [popoverKey]: visible },
+ popoverVisible: { ...prevState.popoverVisible, [popoverKey]: visible },
}));
}
removeAnnotationLayer(annotation) {
- const annotations = this.props.value
- .slice()
- .filter(x => x.name !== annotation.oldName);
+ const annotations = this.props.value.filter(anno => anno !== annotation);
this.props.onChange(annotations);
}
- renderPopover(parent, popoverKey, annotation, error) {
- const id = !annotation ? '_new' : annotation.name;
+ renderPopover(popoverKey, annotation, error) {
+ const id = annotation?.name || '_new';
+
return (
<div id={`annotation-pop-${id}`} data-test="popover-content">
<AnnotationLayer
{...annotation}
- parent={this.refs[parent]}
error={error}
colorScheme={this.props.colorScheme}
vizType={this.props.vizType}
- addAnnotationLayer={this.addAnnotationLayer}
- removeAnnotationLayer={this.removeAnnotationLayer}
- close={() => this.handleVisibleChange(false, popoverKey)}
+ addAnnotationLayer={newAnnotation =>
+ this.addAnnotationLayer(annotation, newAnnotation)
+ }
+ removeAnnotationLayer={() => this.removeAnnotationLayer(annotation)}
+ close={() => {
+ this.handleVisibleChange(false, popoverKey);
+ this.setState({ addedAnnotationIndex: null });
+ }}
/>
</div>
);
@@ -153,6 +155,9 @@ class AnnotationLayerControl extends React.PureComponent {
}
render() {
+ const { addedAnnotationIndex } = this.state;
+ const addedAnnotation = this.props.value[addedAnnotationIndex];
+
const annotations = this.props.value.map((anno, i) => (
<Popover
key={i}
@@ -160,7 +165,6 @@ class AnnotationLayerControl extends React.PureComponent {
placement="right"
title={t('Edit Annotation Layer')}
content={this.renderPopover(
- `overlay-${i}`,
i,
anno,
this.props.annotationError[anno.name],
@@ -183,9 +187,10 @@ class AnnotationLayerControl extends React.PureComponent {
<Popover
trigger="click"
placement="right"
- content={this.renderPopover('overlay-new', addLayerPopoverKey)}
+ content={this.renderPopover(addLayerPopoverKey, addedAnnotation)}
title={t('Add Annotation Layer')}
visible={this.state.popoverVisible[addLayerPopoverKey]}
+ destroyTooltipOnHide
onVisibleChange={visible =>
this.handleVisibleChange(visible, addLayerPopoverKey)
}