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.

Sign up for GitHub

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

Jump to bottom

Fixes to the play slider #5675

Merged
merged 4 commits into from
Aug 22, 2018
Merged

Fixes to the play slider #5675

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b02ca4e
Fixes to the play slider
betodealmeida Aug 18, 2018
bbadfba
Address comments
betodealmeida Aug 20, 2018
5f05a3d
Make CSS rules more strict
betodealmeida Aug 20, 2018
7f1262b
Fix CSS and improve code
betodealmeida Aug 21, 2018
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
7 changes: 5 additions & 2 deletions superset/assets/src/visualizations/PlaySlider.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
.play-slider {
position: absolute;
bottom: -16px;
height: 20px;
width: 100%;
width: 90%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Try wrapping PlaySlider in <div style="position: relative">
It will be better to have the slider at full width rather than 90%

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

}

.slider-selection {
Expand All @@ -21,3 +20,7 @@
color: #b3b3b3;
margin-right: 5px;
}

div.tooltip.tooltip-main.top.in {
margin-left: 0 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to override without using !important?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can use a more specific selector, let me take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kristw, the 3rd-party library bootstrap-slider is setting the left margin directly in the DOM using style=, so it overrules any CSS rules. I only got it to work using !important.

}
11 changes: 9 additions & 2 deletions superset/assets/src/visualizations/PlaySlider.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const propTypes = {
orientation: PropTypes.oneOf(['horizontal', 'vertical']),
reversed: PropTypes.bool,
disabled: PropTypes.bool,
range: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -30,6 +31,7 @@ const defaultProps = {
orientation: 'horizontal',
reversed: false,
disabled: false,
range: true,
};

export default class PlaySlider extends React.PureComponent {
Expand Down Expand Up @@ -87,7 +89,11 @@ export default class PlaySlider extends React.PureComponent {
if (this.props.disabled) {
return;
}
let values = this.props.values.map(value => value + this.increment);
let values = this.props.values;
if (!Array.isArray(values)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The code in if block is not used? Because values is set again right after the block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point, will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be confusing if values is the new one or the old one.
I prefer to avoid using let and try not to mutate variable if not necessary.

const { start, end, step, values, disabled } = this.props;
if (disabled) {
  return;
}
const newValues = (Array.isArray(values) ? values : [values, values + step])
  .map(v => v + this.increment);
// What does cr stand for?
const cr = (newValues[1] > end) ? (newValues[0] - start) : 0;
this.props.onChange(cr === 0 ? newValues : newValues.map(v => v - cr));

Copy link
Member Author

Choose a reason for hiding this comment

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

CR is carriage return — how much the controls move back once they hit the end of the slider. I'll make the code more clear, thanks!

values = [values, values + this.props.step];
}
values = this.props.values.map(value => value + this.increment);
if (values[1] > this.props.end) {
const cr = values[0] - this.props.start;
values = values.map(value => value - cr);
Expand Down Expand Up @@ -116,7 +122,8 @@ export default class PlaySlider extends React.PureComponent {
</Col>
<Col md={11} className="padded">
<ReactBootstrapSlider
value={this.props.values}
value={this.props.range ? this.props.values : this.props.values[0]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use deconstruction for this.props

const { range, values } = this.props;

range={this.props.range}
formatter={this.formatter}
change={this.onChange}
min={this.props.start}
Expand Down
15 changes: 14 additions & 1 deletion superset/assets/src/visualizations/deckgl/AnimatableDeckGLContainer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ const propTypes = {
end: PropTypes.number.isRequired,
step: PropTypes.number.isRequired,
values: PropTypes.array.isRequired,
aggregation: PropTypes.bool,
disabled: PropTypes.bool,
viewport: PropTypes.object.isRequired,
children: PropTypes.node,
};

const defaultProps = {
aggregation: false,
disabled: false,
step: 1,
};
Expand All @@ -26,10 +28,20 @@ export default class AnimatableDeckGLContainer extends React.Component {
const { getLayers, start, end, step, values, disabled, viewport, ...other } = props;
this.state = { values, viewport };
this.other = other;
this.onChange = this.onChange.bind(this);
}
componentWillReceiveProps(nextProps) {
this.setState({ values: nextProps.values, viewport: nextProps.viewport });
}
onChange(newValues) {
let values;
if (!Array.isArray(newValues)) {
values = [newValues, newValues + this.props.step];
} else {
values = newValues;
}
this.setState({ values });
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Avoid negation if not necessary
this.setState({
  values: Array.isArray(newValues) 
    ? newValues
    : [newValues, newValues + this.props.step]
});

}
render() {
const layers = this.props.getLayers(this.state.values);
return (
Expand All @@ -46,7 +58,8 @@ export default class AnimatableDeckGLContainer extends React.Component {
end={this.props.end}
step={this.props.step}
values={this.state.values}
onChange={newValues => this.setState({ values: newValues })}
range={!this.props.aggregation}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use deconstruction from this.props to avoid having this.props.xxx multiple time.

const { end, step, aggregation } = this.props;
const { values } = this.state;

onChange={this.onChange}
/>
}
{this.props.children}
Expand Down
1 change: 1 addition & 0 deletions superset/assets/src/visualizations/deckgl/layers/screengrid.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class DeckGLScreenGrid extends React.PureComponent {
mapboxApiAccessToken={this.props.payload.data.mapboxApiKey}
mapStyle={this.props.slice.formData.mapbox_style}
setControlValue={this.props.setControlValue}
aggregation
/>
</div>
);
Expand Down