-
Notifications
You must be signed in to change notification settings - Fork 121
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
fix(tickformatter): add timeZone to tickFormatter #430
fix(tickformatter): add timeZone to tickFormatter #430
Conversation
to correctly handle a the tick formatting on time based series, we need to share the timeZone parameter to the tickFormatter. When the tickFormatter is an external function, the user can manually configure that parameter, but for the niceTimeFormatter we don't have that option already. Instead of adding a breaking change I've added an optional parameter to the tickFormat props to share the configured timeZone parameter fix elastic#427
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
==========================================
- Coverage 97.85% 97.77% -0.09%
==========================================
Files 39 40 +1
Lines 2896 2921 +25
Branches 701 706 +5
==========================================
+ Hits 2834 2856 +22
- Misses 55 58 +3
Partials 7 7
Continue to review full report at Codecov.
|
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, only a few suggestions.
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.
This looks clear to me - Is there a way for a user to break this with bad input? I see that timeZone is of type string for input like 'utc+3'. Doees moment.js just handle bad timezones like timeZone: 'blarg'
?
@rshen91 with the addition of |
@nickofthyme @rshen91 do we want to catch that moment exception and throw just a more "nicer" console.warn ? |
No I don't think that is needed. Wylie pointed out our lack of error catching, which he's right. I think it's a bigger issue to handle all bad inputs to chart "nicely" in both dev and production. One thing we could add for this is an enum but that would limit the flexibility of kibana to just pass a string. I think |
🎉 This PR is included in version 13.5.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [13.5.6](elastic/elastic-charts@v13.5.5...v13.5.6) (2019-10-22) ### Bug Fixes * **tickformatter:** add timeZone to tickFormatter ([opensearch-project#430](elastic/elastic-charts#430)) ([347eacb](elastic/elastic-charts@347eacb)), closes [opensearch-project#427](elastic/elastic-charts#427)
Summary
to correctly handle a the tick formatting on time based series, we need to share the timeZone
parameter to the tickFormatter. When the tickFormatter is an external function, the user can
manually configure that parameter, but for the niceTimeFormatter we don't have that option already.
Instead of adding a breaking change I've added an optional parameter to the tickFormat props to
share the configured timeZone parameter
fix #427
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.src/index.ts
(and stories only import from../src
except for test data & storybook)[ ] This was checked for cross-browser compatibility, including a check against IE11