Skip to content
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

[Explore] Streamlined metric definitions for SQLA and Druid #4663

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions superset/assets/.eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,8 @@
"react/no-unescaped-entities": 0,
"react/no-unused-prop-types": 0,
"react/no-string-refs": 0,
"indent": 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these are due to the new eslint version which flagged them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah exactly

"no-multi-spaces": 0,
"padded-blocks": 0,
}
}
10 changes: 7 additions & 3 deletions superset/assets/javascripts/components/ColumnTypeLabel.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@ import React from 'react';
import PropTypes from 'prop-types';

const propTypes = {
type: PropTypes.string.isRequired,
type: PropTypes.string,
};

export default function ColumnTypeLabel({ type }) {
let stringIcon = '';
if (type === '' || type === 'expression') {
if (typeof type !== 'string') {
stringIcon = '?';
} else if (type === '' || type === 'expression') {
stringIcon = 'ƒ';
} else if (type === 'aggregate') {
stringIcon = 'AGG';
} else if (type.match(/.*char.*/i) || type.match(/string.*/i) || type.match(/.*text.*/i)) {
stringIcon = 'ABC';
} else if (type.match(/.*int.*/i) || type === 'LONG' || type === 'DOUBLE') {
} else if (type.match(/.*int.*/i) || type === 'LONG' || type === 'DOUBLE' || type === 'FLOAT') {
stringIcon = '#';
} else if (type.match(/.*bool.*/i)) {
stringIcon = 'T/F';
Expand Down
6 changes: 3 additions & 3 deletions superset/assets/javascripts/components/OnPasteSelect.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ export default class OnPasteSelect extends React.Component {
render() {
const SelectComponent = this.props.selectWrap;
const refFunc = (ref) => {
if (this.props.ref) {
this.props.ref(ref);
if (this.props.refFunc) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay for readability!

this.props.refFunc(ref);
}
this.pasteInput = ref;
};
Expand All @@ -68,7 +68,7 @@ export default class OnPasteSelect extends React.Component {
OnPasteSelect.propTypes = {
separator: PropTypes.string.isRequired,
selectWrap: PropTypes.func.isRequired,
ref: PropTypes.func,
refFunc: PropTypes.func,
onChange: PropTypes.func.isRequired,
valueKey: PropTypes.string.isRequired,
labelKey: PropTypes.string.isRequired,
Expand Down
1 change: 1 addition & 0 deletions superset/assets/javascripts/dashboard/reducers.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable camelcase */
import { combineReducers } from 'redux';
import d3 from 'd3';
import shortid from 'shortid';
Expand Down
32 changes: 32 additions & 0 deletions superset/assets/javascripts/explore/AdhocMetric.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
export default class AdhocMetric {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dig the class approach!

constructor(adhocMetric) {
this.column = adhocMetric.column;
this.aggregate = adhocMetric.aggregate;
this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label);
this.fromFormData = !!adhocMetric.optionName;
this.label = this.hasCustomLabel ? adhocMetric.label : this.getDefaultLabel();

this.optionName = adhocMetric.optionName ||
`metric_${Math.random().toString(36).substring(2, 15)}_${Math.random().toString(36).substring(2, 15)}`;
}

getDefaultLabel() {
return `${this.aggregate || ''}(${(this.column && this.column.column_name) || ''})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this.aggregate be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't be often, but in the case that someone clears the aggregate input in the edit popover, and then goes to edit the title, the default title they would see without this logic would be like null(column_name), which seems like a bad user experience. Adding this logic would make the default title appear as (column_name)

}

duplicateWith(nextFields) {
return new AdhocMetric({
...this,
...nextFields,
});
}

equals(adhocMetric) {
return adhocMetric.label === this.label &&
adhocMetric.aggregate === this.aggregate &&
(
(adhocMetric.column && adhocMetric.column.column_name) ===
(this.column && this.column.column_name)
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import React from 'react';
import PropTypes from 'prop-types';
import { Button, ControlLabel, FormGroup, Popover } from 'react-bootstrap';
import VirtualizedSelect from 'react-virtualized-select';

import { AGGREGATES } from '../constants';
import { t } from '../../locales';
import VirtualizedRendererWrap from '../../components/VirtualizedRendererWrap';
import OnPasteSelect from '../../components/OnPasteSelect';
import AdhocMetricEditPopoverTitle from './AdhocMetricEditPopoverTitle';
import columnType from '../propTypes/columnType';
import AdhocMetric from '../AdhocMetric';
import ColumnOption from '../../components/ColumnOption';

const propTypes = {
adhocMetric: PropTypes.instanceOf(AdhocMetric).isRequired,
onChange: PropTypes.func.isRequired,
onClose: PropTypes.func.isRequired,
columns: PropTypes.arrayOf(columnType),
datasourceType: PropTypes.string,
};

const defaultProps = {
columns: [],
};

export default class AdhocMetricEditPopover extends React.Component {
constructor(props) {
super(props);
this.onSave = this.onSave.bind(this);
this.onColumnChange = this.onColumnChange.bind(this);
this.onAggregateChange = this.onAggregateChange.bind(this);
this.onLabelChange = this.onLabelChange.bind(this);
this.state = { adhocMetric: this.props.adhocMetric };
this.selectProps = {
multi: false,
name: 'select-column',
labelKey: 'label',
autosize: false,
clearable: true,
selectWrap: VirtualizedSelect,
};
}

onSave() {
this.props.onChange(this.state.adhocMetric);
this.props.onClose();
}

onColumnChange(column) {
this.setState({ adhocMetric: this.state.adhocMetric.duplicateWith({ column }) });
}

onAggregateChange(aggregate) {
// we construct this object explicitly to overwrite the value in the case aggregate is null
this.setState({
adhocMetric: this.state.adhocMetric.duplicateWith({
aggregate: aggregate && aggregate.aggregate,
}),
});
}

onLabelChange(e) {
this.setState({
adhocMetric: this.state.adhocMetric.duplicateWith({
label: e.target.value, hasCustomLabel: true,
}),
});
}

render() {
const { adhocMetric, columns, onChange, onClose, datasourceType, ...popoverProps } = this.props;

const columnSelectProps = {
placeholder: t('%s column(s)', columns.length),
options: columns,
value: this.state.adhocMetric.column && this.state.adhocMetric.column.column_name,
onChange: this.onColumnChange,
optionRenderer: VirtualizedRendererWrap(option => (
<ColumnOption column={option} showType />
)),
valueRenderer: column => column.column_name,
valueKey: 'column_name',
};

const aggregateSelectProps = {
placeholder: t('%s aggregates(s)', Object.keys(AGGREGATES).length),
options: Object.keys(AGGREGATES).map(aggregate => ({ aggregate })),
value: this.state.adhocMetric.aggregate,
onChange: this.onAggregateChange,
optionRenderer: VirtualizedRendererWrap(aggregate => aggregate.aggregate),
valueRenderer: aggregate => aggregate.aggregate,
valueKey: 'aggregate',
};

if (this.props.datasourceType === 'druid') {
aggregateSelectProps.options = aggregateSelectProps.options.filter((
option => option.aggregate !== 'AVG'
));
}

const popoverTitle = (
<AdhocMetricEditPopoverTitle
adhocMetric={this.state.adhocMetric}
onChange={this.onLabelChange}
/>
);

const stateIsValid = this.state.adhocMetric.column && this.state.adhocMetric.aggregate;
const hasUnsavedChanges = this.state.adhocMetric.equals(this.props.adhocMetric);

return (
<Popover
id="metrics-edit-popover"
title={popoverTitle}
{...popoverProps}
>
<FormGroup>
<ControlLabel><strong>column</strong></ControlLabel>
<OnPasteSelect {...this.selectProps} {...columnSelectProps} />
</FormGroup>
<FormGroup>
<ControlLabel><strong>aggregate</strong></ControlLabel>
<OnPasteSelect {...this.selectProps} {...aggregateSelectProps} />
</FormGroup>
<Button
disabled={!stateIsValid}
bsStyle={(hasUnsavedChanges || !stateIsValid) ? 'default' : 'primary'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: we've veen using bsSize="small" throughout the app

bsSize="small"
className="m-r-5"
onClick={this.onSave}
>
Save
</Button>
<Button bsSize="small" onClick={this.props.onClose}>Close</Button>
</Popover>
);
}
}
AdhocMetricEditPopover.propTypes = propTypes;
AdhocMetricEditPopover.defaultProps = defaultProps;
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import React from 'react';
import PropTypes from 'prop-types';
import { FormControl, OverlayTrigger, Tooltip } from 'react-bootstrap';
import AdhocMetric from '../AdhocMetric';

const propTypes = {
adhocMetric: PropTypes.instanceOf(AdhocMetric),
onChange: PropTypes.func.isRequired,
};

export default class AdhocMetricEditPopoverTitle extends React.Component {
constructor(props) {
super(props);
this.onMouseOver = this.onMouseOver.bind(this);
this.onMouseOut = this.onMouseOut.bind(this);
this.onClick = this.onClick.bind(this);
this.onBlur = this.onBlur.bind(this);
this.state = {
isHovered: false,
isEditable: false,
};
}

onMouseOver() {
this.setState({ isHovered: true });
}

onMouseOut() {
this.setState({ isHovered: false });
}

onClick() {
this.setState({ isEditable: true });
}

onBlur() {
this.setState({ isEditable: false });
}

refFunc(ref) {
if (ref) {
ref.focus();
}
}

render() {
const { adhocMetric, onChange } = this.props;

const editPrompt = <Tooltip id="edit-metric-label-tooltip">Click to edit label</Tooltip>;

return (
<OverlayTrigger
placement="top"
overlay={editPrompt}
onMouseOver={this.onMouseOver}
onMouseOut={this.onMouseOut}
onClick={this.onClick}
onBlur={this.onBlur}
>
{this.state.isEditable ?
<FormControl
className="metric-edit-popover-label-input"
type="text"
placeholder={adhocMetric.label}
value={adhocMetric.hasCustomLabel ? adhocMetric.label : ''}
onChange={onChange}
inputRef={this.refFunc}
/> :
<span>
{adhocMetric.hasCustomLabel ? adhocMetric.label : 'My Metric'}
&nbsp;
<i className="fa fa-pencil" style={{ color: this.state.isHovered ? 'black' : 'grey' }} />
</span>
}
</OverlayTrigger>
);
}
}
AdhocMetricEditPopoverTitle.propTypes = propTypes;
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import React from 'react';
import PropTypes from 'prop-types';
import { Label, OverlayTrigger } from 'react-bootstrap';

import AdhocMetricEditPopover from './AdhocMetricEditPopover';
import AdhocMetric from '../AdhocMetric';
import columnType from '../propTypes/columnType';

const propTypes = {
adhocMetric: PropTypes.instanceOf(AdhocMetric),
onMetricEdit: PropTypes.func.isRequired,
columns: PropTypes.arrayOf(columnType),
multi: PropTypes.bool,
datasourceType: PropTypes.string,
};

export default class AdhocMetricOption extends React.PureComponent {
constructor(props) {
super(props);
this.closeMetricEditOverlay = this.closeMetricEditOverlay.bind(this);
}

closeMetricEditOverlay() {
this.refs.overlay.hide();
}

render() {
const { adhocMetric } = this.props;
const overlay = (
<AdhocMetricEditPopover
adhocMetric={adhocMetric}
onChange={this.props.onMetricEdit}
onClose={this.closeMetricEditOverlay}
columns={this.props.columns}
datasourceType={this.props.datasourceType}
/>
);

return (
<OverlayTrigger
ref="overlay"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a function? (not sure what OverlayTrigger expects)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref can be either a function, or if you're just setting an instance varialble, this is shorthand for:

ref={(ref) => { this.overlay = ref; }}

placement="right"
trigger="click"
disabled
overlay={overlay}
rootClose
defaultOverlayShown={!adhocMetric.fromFormData}
>
<Label style={{ margin: this.props.multi ? 0 : 3, cursor: 'pointer' }}>
<div onMouseDownCapture={(e) => { e.stopPropagation(); }}>
<span className="m-r-5 option-label">
{adhocMetric.label}
</span>
</div>
</Label>
</OverlayTrigger>
);
}
}
AdhocMetricOption.propTypes = propTypes;
22 changes: 22 additions & 0 deletions superset/assets/javascripts/explore/components/AggregateOption.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to declare more than one component in a module if the component is only used once in that one module. Point being to avoid juggling with lots of small files when possible, the components can be refactored out to its own module when it grows to be used in more than one component.

I've been guilty of the other extreme (very long modules) so I'm ok with this if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do prefer more files in general.. I think in this instance it makes sense so all the *Option components are declared in the same way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ small files. it simplifies everything vastly if everything has it's own file.

import PropTypes from 'prop-types';

import ColumnTypeLabel from '../../components/ColumnTypeLabel';
import aggregateOptionType from '../propTypes/aggregateOptionType';

const propTypes = {
aggregate: aggregateOptionType,
showType: PropTypes.bool,
};

export default function AggregateOption({ aggregate, showType }) {
return (
<div>
{showType && <ColumnTypeLabel type="aggregate" />}
<span className="m-r-5 option-label">
{aggregate.aggregate_name}
</span>
</div>
);
}
AggregateOption.propTypes = propTypes;
Loading