-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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(explore): Time picker enhancement follow up #12208
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12208 +/- ##
==========================================
+ Coverage 63.00% 67.18% +4.17%
==========================================
Files 996 1002 +6
Lines 49192 49229 +37
Branches 4993 5000 +7
==========================================
+ Hits 30995 33074 +2079
+ Misses 17997 16032 -1965
+ Partials 200 123 -77
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
231c3ca
to
327b55b
Compare
Please take a care of flaky tests |
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.
Mostly LGTM. A couple of more suggestions on code structure.
Do you plan to add backend validation back in any shape or form? E.g., add a loading state when clicking on "Apply" and block the popover from closing when validation fails.
const iso8601 = String.raw`\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(?:\.\d+)?(?:(?:[+-]\d\d:\d\d)|Z)?`; | ||
const datetimeConstant = String.raw`TODAY|NOW`; | ||
const grainValue = String.raw`[+-]?[1-9][0-9]*`; | ||
const grain = String.raw`YEAR|QUARTER|MONTH|WEEK|DAY|HOUR|MINUTE|SECOND`; |
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.
Any reason to not use UPPER_CASE for these constants?
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.
Use UPPER CASE just for constant looks well. In fact, case insensitive in python datetime expression parser, in other words 'YEAR' == 'year' == 'YEAR' for backend datetime parser.
superset-frontend/src/explore/components/controls/DateFilterControl/AdvancedFrame.tsx
Outdated
Show resolved
Hide resolved
970f109
to
4e7a391
Compare
I will keep an eye on this flaky test |
* refactor layout * WIP * fix typing * styling * fix lint * frontend IT * rename variable * added quarter * typos * refine code structure
* refactor layout * WIP * fix typing * styling * fix lint * frontend IT * rename variable * added quarter * typos * refine code structure
SUMMARY
follow up PR
UI Design
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
Last
Previous
Custom
Advanced
No filter
TEST PLAN
Added cypress for the time picker
ADDITIONAL INFORMATION