Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add custom formatting for Date Nanos Format #42445

Merged
merged 5 commits into from
Aug 9, 2019
Merged

Add custom formatting for Date Nanos Format #42445

merged 5 commits into from
Aug 9, 2019

Conversation

bartvanremortele
Copy link
Contributor

@bartvanremortele bartvanremortele commented Aug 1, 2019 •
edited by kertal
Loading

Summary

Fixes: #38831

Adds custom formatting (FieldFormatEditor) for Date Nanos by extending the Date's field formatter and mounting it

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@kertal
Copy link
Member

kertal commented Aug 1, 2019

jenkins test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Aug 2, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

kertal
kertal requested changes Aug 2, 2019
View reviewed changes
Copy link
Member

@kertal kertal left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, tested it locally in Chrome, works. We kindly would request some changes to the Samples table, it would be neat if it offered some date_nanos content. Currently you're extending the DateFormatEditor. Would you be so kind to adapt it by copying the DateFormatEditor component code (remove extending). Then it would be easy to implement some date_nanos related changes. That would be awesome, thx a lot for you effort!

@bartvanremortele bartvanremortele requested review from a team as code owners August 6, 2019 14:18
@bartvanremortele bartvanremortele requested review from a team August 6, 2019 14:18
@bartvanremortele bartvanremortele requested a review from a team as a code owner August 6, 2019 14:18
@bartvanremortele bartvanremortele force-pushed the enhancement/custom-formatting-date-nanos branch from e40aa82 to 34c4e9d Compare August 6, 2019 14:23
@jbudz jbudz requested review from kertal and removed request for a team August 6, 2019 14:30
@bartvanremortele
Copy link
Contributor Author

bartvanremortele commented Aug 6, 2019 •
edited
Loading

@kertal duplicated the code from DateFormatEditor Ihowever due to the way the samples are set-up within the DefaultFormatEditor the samples will not show the input as date nano's unless I work around the conversion of the sample input. The way it's set-up right now is that it accepts an array of input samples and converts those by doing a lookup for it's converter. But since the input is date nanos and the converter expects Date objects as input, the nanoseconds will always be lost when displaying them in the samples table. This is due to Javascript's date object only supporting up to milliseconds. Moment.js also does not have support for nanoseconds except for formatting them as a date.

A possible workaround for this could be to not use the sample input conversion and provide the samples directly to the <FormatEditorSamples ../> component but I'm not sure if that is the right way to go since they would be hardcoded.

@bartvanremortele
Copy link
Contributor Author

You can disregard my last comment, I figured it out. I'll add samples and try to add the tests as well.

@bartvanremortele
Copy link
Contributor Author

You can disregard my last comment, I figured it out. I'll add samples and try to add the tests as well.

Ready for review again

@kertal
Copy link
Member

kertal commented Aug 6, 2019

jest, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal
Copy link
Member

kertal commented Aug 7, 2019

I will review this PR as soon as a can, looks very good at the first sight, thank you for you patiences

kertal
kertal approved these changes Aug 9, 2019
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, tested locally in Chrome

@kertal
Copy link
Member

kertal commented Aug 9, 2019

Nice work! thanks a lot! i will update the PR, since it's outdated, do IE11 check, just to be sure, and then I'll merge it

@kertal kertal self-assigned this Aug 9, 2019
@kertal kertal merged commit 6637bbe into elastic:master Aug 9, 2019
@kertal kertal mentioned this pull request Aug 9, 2019
Merged
kertal pushed a commit to kertal/kibana that referenced this pull request Aug 9, 2019
@bartvanremortele @kertal
Add custom formatting for Date Nanos Format (elastic#42445)
5a586ad 
* Add custom formatting for Date Nanos format

* Add date nanoseconds samples

* Add jest test for DateNanosFormatEditor
kertal added a commit that referenced this pull request Aug 9, 2019
@kertal
Add custom formatting for Date Nanos Format (#42445) (#43012)
a2dd2d0 
* Add custom formatting for Date Nanos format

* Add date nanoseconds samples

* Add jest test for DateNanosFormatEditor
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into feature/rollu…
1394f75 
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (306 commits)
  [ML] Adding job overrides to the module setup endpoint (elastic#42946)
  [APM] Fix missing RUM url (elastic#42940)
  close socket timeouts without message (elastic#42456)
  Upgrade elastic/charts to 8.1.6 (elastic#42518)
  [ML] Delete old AngularJS data visualizer and refactor folders (elastic#42962)
  Add custom formatting for Date Nanos Format (elastic#42445)
  [Vega] Shim new platform - vega_fn.js -> vega_fn.js , use ExpressionFunction (elastic#42582)
  add socket.getPeerCertificate to KibanaRequest (elastic#42929)
  [Automation] ISTANBUL PRESET PATH is not working fine with constructor(private foo) (elastic#42683)
  [ML] Data frames: Updated stats structure. (elastic#42923)
  [Code] fixed the issue that the repository can not be deleted in some cases. (elastic#42841)
  [kbn-es] Support for passing regex value to ES (elastic#42651)
  Connect to Elasticsearch via SSL when starting kibana with `--ssl` (elastic#42840)
  Add Elasticsearch SSL support for integration tests (elastic#41765)
  Fix duplicate fetch in Visualize (elastic#41204)
  [DOCS] TSVB and Timelion clean up (elastic#42953)
  [Maps] [File upload] Fix maps geojson upload hanging on index step (elastic#42623)
  [APM] Use rounded bucket sizes for transaction distribution (elastic#42830)
  [yarn.lock] consistent resolve domain (elastic#42969)
  [Uptime] [Test] Repurpose unit test assertions to avoid flakiness (elastic#40650)
  ...
@kertal
Copy link
Member

kertal commented Aug 9, 2019

IE11 said yes, so welcome to Kibana 7.4 @bartvanremortel! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

Assignees

@kertal kertal

Labels
release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add custom formatting for Date Nanos Format
5 participants
@bartvanremortele @elasticmachine @kertal @timroes @jbudz