-
Notifications
You must be signed in to change notification settings - Fork 272
fix: Improve big number time format UX #1320
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/BNpFLXvFRF89YLpPDc1r4NgdTPEm |
Codecov Report
@@ Coverage Diff @@
## master #1320 +/- ##
=======================================
Coverage 29.93% 29.94%
=======================================
Files 487 487
Lines 9838 9836 -2
Branches 1648 1648
=======================================
Hits 2945 2945
+ Misses 6656 6654 -2
Partials 237 237
Continue to review full report at Codecov.
|
Hello @etr2460 thanks for the improvements and sorry for defaulting to a non-standard time format. Changes are completely fine with me. |
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.
YYYY-MM-DD for the win!
But shouldn't the default be Smart Date? I don't think we even need to provide a default (empty value will be treated like Smart Date, hopefully) if the control is going to be displayed always. |
Hmm, if that's true then i agree removing it is correct. I'll make the change |
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.
LGTM
🐛 Bug Fix
#1278 introduced the ability to show a timestamp for the value of big number, but as a result it overrode the default time format for the tooltip in big number trendline charts to a somewhat obscure (for americans)
%d-%m-%Y %H:%M:%S
format. This PR:'%Y-%m-%d %H:%M:%S'
to: @krsnik93 @villebro @ktmud