Fixes to the play slider #5675

betodealmeida · 2018-08-18T19:54:54Z

Some fixes to the CSS:

Removed negative bottom position, since the slider was disappearing.
Made with smaller so it looks better (see screenshot).
Fixed tooltip position that was off-centered to the left.

More importantly, added a props called aggregation to AnimatableDeckGLContainer. When true, the play slider will not allow selecting a wider period than the time granularity. This means that for the scatter plot or GeoJSON visualizations (which show just features) the user can select a wider period and see all the features combined. But for screengrid this doesn't make sense, so it users will be able to only step through the granularity.

codecov-io · 2018-08-18T20:13:49Z

Codecov Report

Merging #5675 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5675      +/-   ##
==========================================
- Coverage   63.49%   63.48%   -0.02%     
==========================================
  Files         360      360              
  Lines       22889    22930      +41     
  Branches     2549     2559      +10     
==========================================
+ Hits        14534    14556      +22     
- Misses       8340     8359      +19     
  Partials       15       15

Impacted Files	Coverage Δ
...ts/src/visualizations/deckgl/layers/screengrid.jsx	`0% <ø> (ø)`	⬆️
...isualizations/deckgl/AnimatableDeckGLContainer.jsx	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/PlaySlider.jsx	`0% <0%> (ø)`	⬆️
.../assets/src/visualizations/parallel_coordinates.js	`0% <0%> (ø)`	⬆️
.../assets/src/SqlLab/components/TabbedSqlEditors.jsx	`90.99% <0%> (ø)`	⬆️
...hboard/components/gridComponents/new/NewHeader.jsx	`100% <0%> (ø)`	⬆️
...dashboard/components/gridComponents/new/NewRow.jsx	`100% <0%> (ø)`	⬆️
superset/assets/src/explore/controls.jsx	`45.92% <0%> (ø)`	⬆️
...hboard/components/gridComponents/new/NewColumn.jsx	`100% <0%> (ø)`	⬆️
.../assets/src/SqlLab/components/SqlEditorLeftBar.jsx	`92.52% <0%> (ø)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d15d91...7f1262b. Read the comment docs.

kristw · 2018-08-19T06:00:13Z

superset/assets/src/visualizations/deckgl/AnimatableDeckGLContainer.jsx

+    } else {
+      values = newValues;
+    }
+    this.setState({ values });


Avoid negation if not necessary

this.setState({ values: Array.isArray(newValues) ? newValues : [newValues, newValues + this.props.step] });

kristw · 2018-08-19T07:24:56Z

superset/assets/src/visualizations/PlaySlider.css

@@ -21,3 +20,7 @@
    color: #b3b3b3;
    margin-right: 5px;
 }
+
+div.tooltip.tooltip-main.top.in {
+    margin-left: 0 !important;


Is there a way to override without using !important?

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

@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.

kristw · 2018-08-19T07:26:01Z

superset/assets/src/visualizations/PlaySlider.jsx

@@ -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)) {


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

Ah, good point, will fix.

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));

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!

kristw · 2018-08-19T07:28:07Z

superset/assets/src/visualizations/deckgl/AnimatableDeckGLContainer.jsx

@@ -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}


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

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

kristw · 2018-08-19T07:29:14Z

superset/assets/src/visualizations/PlaySlider.jsx

@@ -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]}


Use deconstruction for this.props

const { range, values } = this.props;

kristw · 2018-08-21T05:57:19Z

superset/assets/src/visualizations/PlaySlider.css

    height: 20px;
-    width: 100%;
+    width: 90%;


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

betodealmeida · 2018-08-21T18:56:36Z

@kristw this is how it looks now:

mistercrunch · 2018-08-21T20:40:03Z

LGTM

kristw · 2018-08-21T20:45:10Z

LGTM

* Fixes to the play slider * Address comments * Make CSS rules more strict * Fix CSS and improve code (cherry picked from commit bcc0954)

* Fixes to the play slider * Address comments * Make CSS rules more strict * Fix CSS and improve code

Fixes to the play slider

b02ca4e

kristw requested changes Aug 19, 2018

View reviewed changes

betodealmeida added 2 commits August 20, 2018 08:29

Address comments

bbadfba

Make CSS rules more strict

5f05a3d

kristw reviewed Aug 21, 2018

View reviewed changes

Fix CSS and improve code

7f1262b

mistercrunch merged commit bcc0954 into apache:master Aug 22, 2018

mistercrunch deleted the DPTOOLS-942 branch August 22, 2018 04:01

betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Aug 22, 2018

Fixes to the play slider (apache#5675)

80b784d

* Fixes to the play slider * Address comments * Make CSS rules more strict * Fix CSS and improve code (cherry picked from commit bcc0954)

kristw mentioned this pull request Aug 27, 2018

[WIP] Cherry-pick(s): <description> airbnb/superset-fork#100

Closed

kristw pushed a commit to kristw/incubator-superset that referenced this pull request Aug 27, 2018

Fixes to the play slider (apache#5675)

31108b6

* Fixes to the play slider * Address comments * Make CSS rules more strict * Fix CSS and improve code (cherry picked from commit bcc0954)

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Fixes to the play slider (apache#5675)

199cbb6

* Fixes to the play slider * Address comments * Make CSS rules more strict * Fix CSS and improve code

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to the play slider #5675

Fixes to the play slider #5675

betodealmeida commented Aug 18, 2018 •

edited

Loading

codecov-io commented Aug 18, 2018 •

edited

Loading

kristw Aug 19, 2018

kristw Aug 19, 2018

betodealmeida Aug 20, 2018

betodealmeida Aug 20, 2018

kristw Aug 19, 2018

betodealmeida Aug 19, 2018

kristw Aug 21, 2018

betodealmeida Aug 21, 2018

kristw Aug 19, 2018

kristw Aug 19, 2018

kristw Aug 21, 2018

betodealmeida Aug 21, 2018

betodealmeida commented Aug 21, 2018

mistercrunch commented Aug 21, 2018

kristw commented Aug 21, 2018

Fixes to the play slider #5675

Fixes to the play slider #5675

Conversation

betodealmeida commented Aug 18, 2018 • edited Loading

codecov-io commented Aug 18, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida commented Aug 21, 2018

mistercrunch commented Aug 21, 2018

kristw commented Aug 21, 2018

betodealmeida commented Aug 18, 2018 •

edited

Loading

codecov-io commented Aug 18, 2018 •

edited

Loading