Make time filter more usable #4981

betodealmeida · 2018-05-11T05:35:58Z

This PR addresses #4292. The main change is that it combines the since and until controls into a single one, time_range:

This has a few benefits:

One cannot select a start date that occurs after the end date.
Common time ranges, eg, "last week", can be selected with a single click.

Additionally, there are other improvements to the UX:

Freeform has now been combined with the calendar selector. Users can type a date, a freeform string ("last Sunday") or use the calendar. Pressing enter closes the dialog.
Selecting a common range, eg, "last week", will update both calendars with the corresponding start/end dates. This allows quickly selecting a range and then fine tuning it, or switching from relative to fixed.
Choosing a date from the calendar closes the calendar.

(@vylc, note that instead of using "Relative" and "Fixed/Freeform" like in your mocks, I used "Range" and "Start/End". This is because the latter can also be relative, since freeform can take strings like "last Sunday", eg. I'm open to better naming suggestions.)

Tests

Opening saved slices and bookmarks with only since and until work.
Filter box works.
Unit tests were added.

…TOOLS-324_ui_for_time_filter

codecov-io · 2018-05-11T23:15:21Z

Codecov Report

Merging #4981 into master will decrease coverage by 15.91%.
The diff coverage is 68.46%.

@@             Coverage Diff             @@
##           master    #4981       +/-   ##
===========================================
- Coverage   77.15%   61.24%   -15.92%     
===========================================
  Files          44      373      +329     
  Lines        8892    23600    +14708     
  Branches        0     2735     +2735     
===========================================
+ Hits         6861    14453     +7592     
- Misses       2031     9132     +7101     
- Partials        0       15       +15

Impacted Files	Coverage Δ
superset/assets/src/explore/controls.jsx	`47.32% <ø> (ø)`
superset/assets/src/explore/visTypes.js	`55.55% <ø> (ø)`
...set/assets/src/dashboard/deprecated/v1/reducers.js	`0% <ø> (ø)`
superset/assets/src/visualizations/filter_box.jsx	`27.27% <0%> (ø)`
superset/viz.py	`77.95% <100%> (-3.4%)`	⬇️
superset/assets/src/logger.js	`95.65% <100%> (ø)`
superset/legacy.py	`18.3% <100%> (+3.6%)`	⬆️
superset/views/core.py	`73.11% <100%> (+0.25%)`	⬆️
superset/utils.py	`89.16% <100%> (+0.95%)`	⬆️
.../explore/components/controls/DateFilterControl.jsx	`60.55% <52.94%> (ø)`
... and 336 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 19ac6e1...03462cb. Read the comment docs.

mistercrunch · 2018-05-14T19:40:47Z

@GabeLoins , mind taking a look into this as this falls under similar as your recent work?

mistercrunch · 2018-05-14T19:43:07Z

For UI consistency with the work on MetricsControl and FiltersControl I'd go with tabs over pills.

gabe-lyons · 2018-05-14T19:56:22Z

I'll take a look. Plus plus for using react-bootstrap Tabs for switching between Range and Start/End for visual consistency.

We have also been thinking about ways to improve this section on our side. For future work, if you're interested, I think it'd be great to collapse all three components in the Time section into a single pill that lets you configure all this stuff

gabe-lyons · 2018-05-14T19:59:26Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

 };

+


accidental newline?

gabe-lyons · 2018-05-14T20:21:00Z

Another thought- for the metric and filter popovers I had the state in the popover only persist when the user clicked Save. I prefer the save functionality so if the user makes a mistake or changes their mind its easy to get back to the original state. For these benefits, consistency, I think it would be an improvement to include a concept of saving to this popover

gabe-lyons · 2018-05-14T20:00:12Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

 export default class DateFilterControl extends React.Component {
  constructor(props) {
    super(props);
-    const value = props.value || '';
+    const value = props.value || defaultProps.value;


isn't this redundant of how react already deals with defaultProps?

Yeah, I was unsure about this and was afraid of breaking things. I assume it's protecting cases where value is explicitly set to null?

PropTypes (if the field is required) will prevent against null being passed in- I'd say drop the || defaultProps.value.

If the field can't be required, I'd still say its not needed- controls across explore take in the value prop at face value and don't do this check. For it to create a problem someone would have to start setting value as null in a higher level explore component which I think would be a bigger issue.

gabe-lyons · 2018-05-14T20:13:34Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

+      // for start:end(includes freeform)
+      since: DEFAULT_SINCE,
+      until: DEFAULT_UNTIL,
+      isFreeform: {},


I feel like this value should be a boolean if its prefixed with is?

What if we initialize it as:

isFreeform: { since: false, until: false }

I can also rename it to freeformFields.

gabe-lyons · 2018-05-14T20:28:54Z

superset/assets/package.json

@@ -88,6 +88,7 @@
    "react-addons-shallow-compare": "^15.4.2",
    "react-alert": "^2.3.0",
    "react-bootstrap": "^0.31.5",
+    "react-bootstrap-datetimepicker": "0.0.22",


just noticed this component is deprecated, should we switch to the non-deprecated package?

https://github.com/YouCanBookMe/react-datetime

Let me take a look at how much effort is needed to use that one... I remember testing it when I was looking for calendar components, and it didn't work for some cases but TBH I don't remember the details.

gabe-lyons · 2018-05-14T20:30:02Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

+const INVALID_DATE_MESSAGE = 'Invalid date';
+const SEPARATOR = ' : ';
+const FREEFORM_TOOLTIP = t(
+  'Superset supports smart date parsing. Strings like `last sunday` or ' +


[honest question] is this functionality worth supporting? It seems like if I wasn't a power user I wouldn't know which keywords work and which don't. For example, would "2 weeks ago" work? What about "the quarter before last"

Even if we decided we're better off without it, it'd be hard to deprecate (would break charts).

Yeah, without feedback informing if the input is valid this doesn't seem very usable. We could do a backend call and show it in red if not supported?

gabe-lyons · 2018-05-14T20:42:34Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

-      this.state.dttm = value;
-      this.state.type = 'fix';
+    if (value.indexOf(SEPARATOR) >= 0) {
+      this.state.type = 'startend';


Since put this string in a constant at the top of this file it seems like it'd be best if you referenced it from the constant as well? Otherwise why use a constant?

Good point, let me fix this.

gabe-lyons · 2018-05-14T20:47:33Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

+    const grainOptions = TIME_GRAIN_OPTIONS.map((grain, index) => (
+      <MenuItem
+        onSelect={this.setCustomRange.bind(this, 'grain')}
+        key={'grain-' + index}


generally using index as a key is considered an antipattern (https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318)

also style nit: I don't think you need to add that grain- prefix

Ah, #TIL. Will fix, though in this case it should be safe (no reordering or updates in the list).

gabe-lyons · 2018-05-14T20:48:00Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

+        key={'grain-' + index}
+        eventKey={grain}
+        active={grain === this.state.grain}
+      >{grain}


nit: {grain} should be on its own line

gabe-lyons · 2018-05-14T20:50:54Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

  }
-  setDatetime(dttm) {
-    this.setState({ dttm: dttm.format().substring(0, 19) });
+  updateRefs() {


this seems like it adds quite a bit of indirection. Would it be possible just to pass value as a prop to these components rather than setting their state via a ref? https://github.com/YouCanBookMe/react-datetime#api

This is needed for freeform text. The calendar component uses moment to parse the string, and if it fails the state is not set. This prevents us from initializing the component saved with freeform values.

gabe-lyons · 2018-05-14T20:52:15Z

superset/assets/src/explore/components/controls/DateFilterControl.jsx

-          </PopoverSection>
+            <Tab eventKey={1} title="Range">
+              <FormGroup>
+                {timeFrames}


maybe this should be its own third tab? It seems like the Range tab is a little busy with the radio buttons and also the dropdown menus

@vylc, any thoughts here? Personally I like this design, since the common ranges and the dropdown serve the same purpose, and allow for a lot of control in a single place.

gabe-lyons · 2018-05-22T00:22:50Z

williaster · 2018-05-22T17:27:45Z

There's some great improvements here! My thoughts on other issues being discussed

++ tabs, we should try hard to move to a world of visual consistency.
++ using the condensed padding that @GabeLoins mentioned, again visual consistency and we purposefully used condensed tabs for his work on metrics/filters.
I think using Ok/cancel is also a good idea, that's a pretty common web paradigm
I think "Range" and "Start/End" are confusing, they are both ranges in the end right? I'm not sure of the best language here, it seems like "range" is really just smart defaults for choosing the start date? will continue to think on this
I like having both the radio buttons + dropdown in the same window pane, IMO it doesn't seem useful to have the radio buttons stand on their own.

michellethomas · 2018-05-22T17:49:13Z

For Start/End it would be nice to have the language make it clear if the range will be inclusive or exclusive. Druid and sqlalchemy data sources do not filter on the date ranges as both inclusive, and it can be especially confusing when looking at sums over a range (like in big number or table viz).

It could be nice if druid/sqlalchemy were consistent, but changing it now would make existing slices incorrect (as people have adjusted to this difference). When we move to using druid sql, this will be consistent.

I'll try to look around more for specific suggestions. Maybe something like Starting after and Up to? It's a little verbose though.

betodealmeida · 2018-05-22T18:28:32Z

Yeah, I'm unsure about the naming. Maybe we could use "simple" and "custom", like the filters?

@michellethomas, how are they different? Is one of them inclusive and the other exclusive?

michellethomas · 2018-05-22T19:14:44Z

Yeah after looking into it, it may be difficult to message because it depends on whether you are using a date or not (and also which engine you are using).

For us using dates (not datetime):
druid start date inclusive / end date exclusive
sqla (with presto) start date exclusive / end date inclusive

sqla tries to be inclusive on both ends and will add a filter like WHERE "ds" >= '2018-05-14 00:00:00' AND "ds" <= '2018-05-18 00:00:00' but it’s getting excluded because of the lexicographic order.

Here's a discussion of the issue. I think it was just dropped because we couldn't find a regression, but there's still an inconsistency we should discuss (maybe out of scope for this PR) https://apache-superset.slack.com/archives/C7G8CG0LR/p1512418244000100

elibrumbaugh · 2018-05-23T01:49:17Z

++ Tabs
++ Condensed padding

I think tabs work great to separate different concepts or types of tasks. As Max pointed out they should be visually consistent. The tabs in the gif look a bit like buttons to me.

I agree with Chris that "Range" and "Start/End" are confusing since they are both types of ranges. I also view the use of "range" in this context as a type of smart default.

A few thoughts:

Regardless of what route we take, we should improve the label clarity.
Could we separate smart defaults into one tab and the user's ability to pick a custom range into another? This would mean moving the current last option from "range" into the second tab.
I think the separation illustrated below would help separate the two types of user desire: 1. give me great defaults and 2. give me the ability to customize them when they don't meet my needs.

For example:

Defaults

[Yesterday]
[Last week]
[Last month]
[Last year]

Custom

[Last v] [#] [days v]
[Start date]
[End date]

An open question I have that effects the order of the tabs is what will people use most often? We should order accordingly.

vylc · 2018-05-23T18:26:33Z

Looks great!
(1) I like the organization that @elibrumbaugh suggested. Can we name the tab "Relative" vs. Defaults. Custom is good.
(2) Re: the inclusive/exclusive that @michellethomas brought up, I think that belongs in an info hover as opposed to trying to name the fields accordingly
(3) Ok/Cancel are good. @GabeLoins do you want to adopt this as well for your metric popover (vs. Save/Close)? For consistency, I'd also move your buttons to the lower right since that's more natural than over to the lower left. Cancel (no color) should be to the left of OK (teal superset color)

betodealmeida · 2018-05-23T18:55:35Z

(1) I like the organization that @elibrumbaugh suggested. Can we name the tab "Relative" vs. Defaults. Custom is good.

@vylc, "relative" is not a good name for the first tab because the second one can also be relative, eg, you could have "last Sunday" as the start and "today" as the end time.

betodealmeida · 2018-07-13T23:10:05Z

Video of modifications:

Screenshots:

williaster · 2018-07-14T03:38:14Z

looking good, thanks for iterating on/incorporating the feedback!

mistercrunch · 2018-07-16T21:27:34Z

LGTM

* Initial work * WIP * WIP * Working * WIP * Still WIP * Frontend done * Working version * Migration working * Migration working * Fix freeform rerender * Remove jquery * Fix filter * Unit tests and lint * Fix py.test * Improve unit tests * Ensure freeform is computed at init * Fix lint * Trying to fix pyfreeze error * Remove freezegun * Address comments * Use tabs instead of pills * Regroup options * WIP * Change type when clicking calendar * Fix CSS * Fix JS lint

betodealmeida added 22 commits April 16, 2018 15:46

Initial work

2aaa349

WIP

42e4f79

WIP

98dd135

Working

25460c5

Merge branch 'master' of github.com:apache/incubator-superset into DP…

b1e2dd5

…TOOLS-324_ui_for_time_filter

WIP

06d9d1b

Still WIP

f8112ed

Frontend done

8b66907

Working version

e41e53d

Migration working

edf0455

Migration working

29c0c6b

Fix freeform rerender

0406a00

Remove jquery

d9e99fa

Fix filter

40c10ab

Unit tests and lint

22689dd

Fix py.test

38175ef

Improve unit tests

1f6a61c

Ensure freeform is computed at init

a028251

Rebase

cb831c3

Fix lint

dc50f6f

Trying to fix pyfreeze error

ab5015e

Remove freezegun

20e6e78

gabe-lyons reviewed May 14, 2018

View reviewed changes

superset/assets/src/explore/components/controls/DateFilterControl.jsx

};

Copy link

Contributor

gabe-lyons May 14, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

accidental newline?

gabe-lyons reviewed May 14, 2018

View reviewed changes

Address comments

80e9c62

betodealmeida added 2 commits May 23, 2018 12:06

Use tabs instead of pills

e6c02e2

Regroup options

cc039f2

hughhhh added the lyft Related to Lyft label May 31, 2018

betodealmeida added 5 commits July 10, 2018 11:15

Merge

128db9f

WIP

b593fa7

Change type when clicking calendar

c917788

Fix CSS

ea99aa9

Rebase

60ead86

Fix JS lint

03462cb

mistercrunch merged commit 4fa4163 into apache:master Jul 16, 2018

mistercrunch deleted the DPTOOLS-324_ui_for_time_filter branch July 16, 2018 21:27

This was referenced Aug 3, 2018

Time filter from older versions break exploring slices from dashboard #5553

Closed

migrate dashboard stored default filters to time range format #5556

Closed

john-bodley mentioned this pull request Oct 10, 2019

[migration] Migrating since/until to time_range #8372

Merged

12 tasks

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

Make time filter more usable #4981

Make time filter more usable #4981

Conversation

betodealmeida commented May 11, 2018

Tests

codecov-io commented May 11, 2018 • edited Loading

Codecov Report

mistercrunch commented May 14, 2018

mistercrunch commented May 14, 2018

gabe-lyons commented May 14, 2018

gabe-lyons May 14, 2018 • edited Loading

Choose a reason for hiding this comment

gabe-lyons commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabe-lyons May 22, 2018 • edited Loading

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

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

gabe-lyons commented May 22, 2018

williaster commented May 22, 2018

michellethomas commented May 22, 2018

betodealmeida commented May 22, 2018

michellethomas commented May 22, 2018

elibrumbaugh commented May 23, 2018 • edited Loading

vylc commented May 23, 2018

betodealmeida commented May 23, 2018

betodealmeida commented Jul 13, 2018

williaster commented Jul 14, 2018

mistercrunch commented Jul 16, 2018

codecov-io commented May 11, 2018 •

edited

Loading

gabe-lyons May 14, 2018 •

edited

Loading

gabe-lyons May 22, 2018 •

edited

Loading

elibrumbaugh commented May 23, 2018 •

edited

Loading