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

Add more predefined options to Date and Date Range parameters #3009

Closed
2 tasks
arikfr opened this issue Oct 28, 2018 · 21 comments · Fixed by #3904
Closed
2 tasks

Add more predefined options to Date and Date Range parameters #3009

arikfr opened this issue Oct 28, 2018 · 21 comments · Fixed by #3904

Comments

@arikfr
Copy link
Member

arikfr commented Oct 28, 2018

The Date Picker we use has support for Presetted Ranges, so we can add some useful short hands like: last month, last week, last 7 days, last 30 days, etc.

  • Add support for picking presetted ranges.
  • Make it possible to pick such range as the default value of this parameter (a more general case of the current "Use today as default value" option)
@ssozonoff
Copy link

Surprised their is not more demand for this. To me its a fundamental requirement of any Dashboarding solution.

@arikfr
Copy link
Member Author

arikfr commented May 7, 2019

The first part (more predefined options) is easy. The tricky part is how to persist those selections as defaults.

One idea I had is to save them as values for start/end, i.e. when you pick "Last 7 Days" the value we pass for param.start/param.end will be something like last_7_days, so we know to convert it to the appropriate timestamps.

A more generic solution will be to pass datetime deltas as values, i.e. when you pick "Last 7 Days" what we pass for start will be "-7 days" and for end "-0 days".

@gabrieldutra
Copy link
Member

gabrieldutra commented Jun 4, 2019

UX/UI
My ideas so far for the second part of it:

  • Add a "Set dynamic" toggle on the parameter. This should make a good discoverability for it, I considered just modify the Parameter Setting dialog, but this would limit the scope to "Editing a query". An example probably not with the best icon that worked as a start:
    date-range-1
  • When clicking on it a dialog will open showing usual options and a Custom (with 'from' and 'to'):
    daterange-dialog
  • When in Dynamic mode it will no longer appear as a DatePicker, but it will show a fixed message containing the selected date range and an option to make it static again.

This is sort of the draft of what I thought for UX/UI, @ranbena can give magic ideas here :)

Implementantion
For implementation, I loved the generic idea of delta values and it's here that the solution for #3053 may be similar. Should the parameter "translation" be backend-wise?

I think it's sort of a workaround doing in the frontend as deep down it loads a static value. Also there's the case for Scheduled Queries.

From the backend I guess my concern is that I haven't seen any of these "translations" in there 😬

@arikfr
Copy link
Member Author

arikfr commented Jun 6, 2019

How about we put the default value setting in the Parameter settings dialog?

Also, we can do the translation from the default value to actual date value when loading the parameter and from this point forward (in the current execution) it will behave as a regular date time picker. No need for special handling when communicating with the backend.

Only special handling on the backend is needed when handling scheduled queries, but we need to see if de-duplicating this logic is worth the extra complexity in implementation.

@ranbena
Copy link
Contributor

ranbena commented Jun 10, 2019

Add a "Set dynamic" toggle on the parameter. This should make a good discoverability for it, I considered just modify the Parameter Setting dialog, but this would limit the scope to "Editing a query". An example probably not with the best icon that worked as a start

I must say I like it!
I would though put the all presets, including the static ones in the modal.
Also, wdyt of having the ⚡icon be a 🔽instead, like the "Refresh 🔽" dashboard button.

@arikfr
Copy link
Member Author

arikfr commented Jun 12, 2019

The default value ("dynamic value") is part of the parameter configuration. Shouldn't it be along with the other parameter configuration values?

@ranbena
Copy link
Contributor

ranbena commented Jun 12, 2019

The default value ("dynamic value") is part of the parameter configuration. Shouldn't it be along with the other parameter configuration values?

I'm misunderstanding sth - why would this be called a "default value"? It's a value like any other, but dynamically set, no?

@gabrieldutra
Copy link
Member

The default value ("dynamic value") is part of the parameter configuration. Shouldn't it be along with the other parameter configuration values?

The problem I see with the parameter configuration is that you limit the scope to when you are editing a query, you wouldn't be able to set it in the Dashboard page for example. An specific button for it felt like an extension to Date Parameters.

@ranbena, I considered the 🔽 it seems to go better with a Dropdown. I did some quick experiments with it and it presented some issues (dropdown inside a date picker 😅), I'll turn those changes into a draft PR so this can be better experienced.

@ranbena
Copy link
Contributor

ranbena commented Jun 12, 2019

@gabrieldutra forget it - leave it as a bolt. It works! ⚡

@gabrieldutra
Copy link
Member

I actually just realized that there's a limitation for this: change and persist the parameter options requires 'edit_query' permissions and that you save the query afterwards. So my idea comes with a few limitations too:

  • The bolt would appear only for the ones who have the permission, with only view this would appear as a fixed value for normal view users. It will be a bit inconsistent as some parameters could appear with the bolt and others wouldn't;
  • Need to replicate the "save query" to Dashboard & widget parameters, this may generate an unwanted behavior since you are actually changing the query Parameter from somewhere else.

Considering this, please lmk if you see other possible conflicting scenarios I haven't.

Alternative options that I considered:

  1. Arik's initial "default value" thought. This will fix by limiting the context of editing that to the query and working similar to the "Today/Now" checkbox. Cons are that it'd be a sort of hidden feature and it can be confusing to have a dashboard parameter value linked to a query. (e.g: you have 'Last week' in a dashboard parameter, how to change that to 'Last month'?)
  2. Change the 'value' prop to be an object with all data needed (static/dynamic, delta value/static dates). This one falls in the backend implementation cons again;
  3. Create a new parameter type. Same as the above, just in a different parameter type.

@arikfr
Copy link
Member Author

arikfr commented Jun 16, 2019

I had the opportunity to discuss this with @ranbena and we came up with the following:

  1. Keep the Dynamic Value setting UI you suggested (the one with the bolt). When a user selects a dynamic value everytime we run the query, the value will keep updating. This to allow for picking something like "Today" in a dashboard that is set to refresh, and to receive updated values on the next day if auto refresh is enabled.
  2. When using a dynamic value, the value we set for the parameter is a special syntax we will create. Something like: dynamic:-7days (the dynamic: prefix is used to make the AI set of if statements that interprets the values simpler).
  3. All preset ranges will be dynamic values. This removes the need to use Ant's presets UI, as it will be confusing which one should be used when.
  4. Resolving of the dynamic values, when used from the UI, will happen on the client side (so we can use the user's timezone). The server will receive the values as simple date/time objects, just like today.
  5. We will need to support resolving dynamic values on the backend for scheduled queries, but this is a simpler use case.
  6. In a date parameter, this dynamic values functionality will replace the current "use today when empty checkbox" feature.
  7. When saving a query when a date/date-range parameter is set to a dynamic value, we will save the dynamic value and as a result the default value becomes dynamic. So no special handling needed.

As for what I mentioned about setting the default value from the parameter settings dialog, I think we should implement this for all parameters but considering 7 above, we can wait with it -- I will open a separate issue for it, once we feel the above is the right approach.

@gabrieldutra , this is a lot to unpack -- if it's unclear, let me know and maybe we can schedule a call to discuss this.

I actually just realized that there's a limitation for this: change and persist the parameter options requires 'edit_query' permissions and that you save the query afterwards. So my idea comes with a few limitations too:

@gabrieldutra why do you need to persist it when just setting the current value?

@arikfr
Copy link
Member Author

arikfr commented Jun 16, 2019

As a side note, here's a nice UX for picking date ranges:

(from Baremetrics)

What I liked here is the fact that next to the range name it shows the actual dates.

@ranbena
Copy link
Contributor

ranbena commented Jun 16, 2019

Not final design, just meant to clarify the vision:

Empty state

Static value selected

Clicked dynamic value button

HelpTrigger: "Guide: Setting Dynamic Dates and Ranges".

Regarding "Custom" option - not a must for this feature.

Dynamic value selected

Open dynamic value options by clicking the bolt.
Open the date picker by clicking the selected value (be the selected value static or dynamic or empty).

@ranbena
Copy link
Contributor

ranbena commented Jun 16, 2019

Edit - added actual date value to selected.

@ranbena
Copy link
Contributor

ranbena commented Jun 16, 2019

@arikfr the only "problem" I see here is that a dynamic value would revoke its original input type.
If this option were to be available for each date input type (Date, Date and Time, Date Range, etc.) then it would actually be overridden by the dynamic value selected: "Today" -> Date input, all the rest ("This week", "This Month", etc.) -> Date Range.

So time won't be at all relevant here (unless we have a "This Minute" option).

@arikfr
Copy link
Member Author

arikfr commented Jun 16, 2019

@ranbena I'm not sure I understand? When it's a date you will onyl have "Today", when it's a data range you will get the other options.

@gabrieldutra
Copy link
Member

gabrieldutra commented Jun 16, 2019

Thanks @arikfr and @ranbena, regarding UI/UX this follows what I had in mind and adds some nice touches to it.

why do you need to persist it when just setting the current value?

That seemed necessary when considering dashboard global parameters, because their values come from the url, all the parameter information (such as the "dynamic" property) come from the query. Also, I was first with the idea that once those "dynamic" values were set, they would stay dynamic on reload.

On a second thought after reading what you said and realizing the current Redash behavior for parameters, there are other ways to solve the above.


I was hoping to have something more consistent before opening the WIP PR, though it will be better to share the possible limitations (Netlify ftw), so I decided to open it from the draft I had in here to shape this feature while checking what can be the issues in the implementation.

@ranbena
Copy link
Contributor

ranbena commented Jun 17, 2019

@ranbena I'm not sure I understand? When it's a date you will onyl have "Today", when it's a data range you will get the other options.

Oh ok, I thought we would always show all dynamic options but your approach makes sense.

@gabrieldutra
Copy link
Member

Edit - added actual date value to selected.

@ranbena Antd's DatePicker doesn't seem to support custom Nodes on the inputs (actually it just supports moment values in this case 😬). Do you have any simpler idea on how to implement this one? (It seems possible to do it by recreating the DatePicker using Calendar, but perhaps you have a simpler idea 😆)

@ranbena
Copy link
Contributor

ranbena commented Jun 22, 2019

@ranbena Antd's DatePicker doesn't seem to support custom Nodes on the inputs (actually it just supports moment values in this case 😬). Do you have any simpler idea on how to implement this one? (It seems possible to do it by recreating the DatePicker using Calendar, but perhaps you have a simpler idea 😆)

How about hijacking the placeholder? 😈

Edit polished-wood-76sfv

@gabrieldutra
Copy link
Member

How about hijacking the placeholder?

Interesting haha, the placeholder passed through my mind too, won't be showing the specific dates, but is the best so far indeed. Thanks! 😁

@gabrieldutra gabrieldutra self-assigned this Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants