fix(ui): adding times in script editor will default start and endtime for timerange queries #16201
Conversation
ghost
requested review from
… for timerange queries
…nge scripts are based on those timeranges
asalem1
force-pushed
the
custom-timerange
branch
from
5c73c06 to
04fa483
Compare
| @@ -15,7 +15,7 @@ import {ErrorHandling} from 'src/shared/decorators/errors' | |||
| interface OwnProps { | |||
| view: View | |||
| check: Partial<Check> | |||
| timeRange: TimeRange | |||
| timeRange: TimeRange | null | |||
There was a problem hiding this comment.
it doesn't look like you are using this prop
There was a problem hiding this comment.
ah you are... but I think you should not be
|
|
||
| export const getActiveTimeRange = (timeRange: TimeRange, queries: array) => { | ||
| if (!queries) { | ||
| return timeRange |
There was a problem hiding this comment.
ooh interesting. I wonder if we should check for when queries.text is empty as well... ?
Would like to take a look at what happens to an empty graph with these changes.
There was a problem hiding this comment.
Hmmm, I wonder if that would ever be possible since there's a minimum criteria to submit a query that gets compiled into the script text
There was a problem hiding this comment.
Could you test it visually with no query before merging please?
There was a problem hiding this comment.
No query = cannot submit without some DOM manipulation (removing the disabled flag from the console)
| const timeZone = state.app.persisted.timeZone | ||
|
|
||
| return { | ||
| endTime: getEndTime(timeRange), | ||
| startTime: getStartTime(timeRange), | ||
| ranges: timeRange, |
There was a problem hiding this comment.
let's keep calling this timeRange to signal to the next programmer that this is the same old timeRange that they know from the rest of the app.
There was a problem hiding this comment.
but we have timeRange in the OwnProps :(
There was a problem hiding this comment.
why are we passing the same prop to the same component twice? What is the difference between them? Which one is redundant?
There was a problem hiding this comment.
The difference between them is that the timeRange refers to the dropdown selection while the ranges refer to the specific ranges in the query script editor
| ) => { | ||
| const range = extent((data as number[]) || []) | ||
| if (!timeRange) { |
There was a problem hiding this comment.
that might happen for a variety of reasons. we don't know what to do if timeRange is undefined.
|
|
||
| export const getActiveTimeRange = (timeRange: TimeRange, queries: array) => { | ||
| if (!queries) { | ||
| return timeRange |
There was a problem hiding this comment.
Could you test it visually with no query before merging please?
… for timerange queries (#16201) fix(ui): adding times in script editor will default start and endtime for timerange queries, compute timeranges based on script and based on widest date range & fix dashboard so that cells that have been saved with timerange scripts are based on those timeranges
Closes #16123
Problem
Currently, graph start and end times were being set based on selections from the dropdown regardless of whether the script editor input manual time ranges.
Solution
Before determining the start and end time, check to see whether a custom time has been input in the script editor