-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Add Dynamic Values to Date and Date Range Parameters #3904
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
Conversation
Idea of implementation updated considering Arik's comment.
Currently applied only for DateRange for comments. A possible refactor that I was thinking is that at some point parameters can be rewritten taking advantage of inheritance (should reduce complexity for the methods like getValue, setValue, toUrlParams as each parameter type will have it's own logic) |
Great to see it in action :) |
471c595
to
7efcd39
Compare
@ranbena @arikfr finally I could sit down and work on this 🙂, the fist part of the feature already has its shape:
I'm pretty happy with the UX/UI result :) Currently the Dynamic information is saved in the Parameter The next is to implement a "Custom option" dialog (should make this reach all of its potential) and then it's a matter of final touches. One note is that I reverted the DateTime Parameter apply only onOk or close thing so this one would be easier (I think it won't be a problem considering #3907). |
client/app/components/dynamic-parameters/DynamicParameters.less
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the Custom Option: It's indeed not a must, but I feel like half of this feature is incomplete without it as all options like "Last X days" could be done there, thus avoiding overload on the dropdown.
This can be delivered separately, if the custom option happens to be worth it, perhaps open a Part 2 to make review a bit easier and explore it without compromising this one?
client/app/components/dynamic-parameters/DynamicParameters.less
Outdated
Show resolved
Hide resolved
But that does mean that there's no way to clear a static value, no? |
I wasn't considering this as required 😅, anyway I updated implementation and put the clear button back in there :) |
Well reminded. I guess so far the pending discussion points in this PR are the "Refresh Schedule" and the size changing for Dynamic Date Ranges. Refresh Schedule Also, when trying to work on that part in #3952, I couldn't get the Refresh Schedule to work (used Python debugger and logs to track). Changing size for Date Ranges
It's possible, different styling can be done by using the two inputs placeholders. One con is that it will differ from Date Parameters (also Date Ranges are very wide 😅). @ranbena any quick ideas for this? So far I'm ok with the changing size thing for styling purposes |
5bd403d
to
34ee589
Compare
In your example it's huge because it's a date range with time. But I wonder if date range with time needs dynamic values? All the dynamic values are date based, so maybe it should be only for date range and date picker? Then the size delta isn't that big.
The timezone issue is indeed tricky in this case :-| With dates only it's less an issue, but still can manifest on day changes. Not sure what to do about this, we can save the current timezone when saving the dynamic values but that's not great. We can decide that there is no scheduling for such queries. Although this opens its own Pandora's box -- what if the query is already scheduled, etc. |
I don't think we should limit the scope of the feature, from a "fixed" value standpoint, yes, it makes no sense for Time pickers (except for the "Now" one). However I think it's convenient to have the Dynamic Values available: you can have a dashboard with a Date Range Time specified and if you want to check out the result for "Last week" you are just a few clicks away. |
@gabrieldutra fair points about keeping it for all types 👍 So what do you think about the option of disabling scheduled queries? |
That's my thinking as well. |
I got a feeling that scheduled queries are not working for queries with parameters, which makes "what if the query is already scheduled" not an issue. But I agree with the "disabling scheduled" for such queries, in backend we can actually use the existing validation to skip execution ( My points are:
|
🤔 I just checked and it does seem like it has no effect / not working. I will check this out.
Let's do it in a separate PR then. |
I'll wrap this with the transition and fix Cypress tests then |
@ranbena, I've updated this with the transition + a fix to a bug (Date Ranges with no previous value + click on "Back to static value" was resulting in |
What type of PR is this? (check all applicable)
Description
Allow Dynamic Values to Date and Date Range Parameters 🎉.
See preview below for details :)
Related Tickets & Documents
Closes #3009
Mobile & Desktop Screenshots/Recordings (if there are UI changes)