-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat: change default time range from sql lab to no filter #9439
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9439 +/- ##
=======================================
Coverage 59.03% 59.03%
=======================================
Files 379 379
Lines 12181 12181
Branches 3010 3010
=======================================
Hits 7191 7191
Misses 4809 4809
Partials 181 181
Continue to review full report at Codecov.
|
ce14124
to
5df6d8c
Compare
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.
one comment around code quality. it's not really blocking, but it seems worth the couple minutes it would take now to address
@@ -139,8 +139,8 @@ class ExploreResultsButton extends React.PureComponent { | |||
datasource: `${data.table_id}__table`, | |||
metrics: [], | |||
groupby: [], | |||
time_range: 'No filter', |
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.
a bit weird that this is just plain text as opposed to null
or something. I don't think fixing that is required right now, but i think moving "No filter" to a constant and using that constant it here would be reasonable. It looks like "No filter" is used in a few other places too (tests, other components) so maybe you could replace those your new constant too?
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.
Agree this is not a good practice. There seems to be many other hard-coded time periods, as well. I am not comfortable with refactoring while changing the default or refactoring just the 'No filter'
. Maybe that refactoring can happen in a later PR?
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.
sure, let's make sure we pull this out as a constant in a future PR then
CI has passed but GitHub still shows pending? Can we merge regardless? |
Change from "100 years go" to "no filter". 100 years ago is basically equivalent to no filter, but has implications on how x-axis is plotted on certain charts (e.g. Big Number).
5df6d8c
to
127aaac
Compare
CATEGORY
SUMMARY
Currently when clicking on "Explore" button in SQL Lab results, it defaults the time range to "100 years ago : infinity", which is basically equivalent to no filter for most datasources. However, it has implication on how the x-axis will be plotted in certain charts (e.g., recently, the Big Number chart introduced a fixed range option).
This PR changes the default to "No filter" so the time range filter can be more properly utilized.
There is also a global default value "Last week", which is applied when users create a new chart from
/chart/add
. We are not changing this default value in this PR.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
When clicking on Explore in SQL Lab:
Before
After
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS
@kristw