[Lens] Improved range formatter #80132

dej611 · 2020-10-12T11:24:39Z

Summary

This PR is a proposal for a new range formatter format.

✨ The new format can be expressed using the params property in the setup payload.
- A new arrow_right template has been added
❓ The range formatter now has some logic to handle specific Infinity scenarios
♻️ Reworked the format information propagation
- Nested formatters are now supported. As for now custom arguments need to be mapped to pass the expression pipeline
✨ Value format field is not enabled in the dimension panel for both base and custom ranges
- both base and custom intervals values can be formatted
🐛 Chart label format now match custom default range label one
🔀 Harmonised formatter with custom labels code

<title>Initial proposal</title>

I would also propose to clean a bit the popover style and remove the mathematical notation from it, just to keep it clean and consistent with the new label format (not implemented yet):

Fixes: #77881

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

…ernative-formatter

dej611 · 2020-10-13T08:55:14Z

cc @AlonaNadler I've used the dash format here, is that ok?
@cchaos what do you think of the simplified range popover without the symbols?

AlonaNadler · 2020-10-13T13:56:08Z

Yes looks good thanks @dej611

cchaos · 2020-10-13T14:07:46Z

I definitely ok with removing those symbols. I would go a bit further to and reduce the width of the inputs a bit. They're quite large for a number input. I'd say like half the size would suffice.

…ernative-formatter

…and custom scenario

elasticmachine · 2020-10-16T10:47:37Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

dej611 · 2020-10-16T12:43:39Z

@elasticmachine merge upstream

flash1293 · 2020-10-22T13:26:54Z

This is the only thing I'm seeing in Lens (installed the logs sample data, no other index patterns around):

It seems like the logic we put in place to remap the field formats to a serialized format don't always work and need a safeguard (x-pack/plugins/lens/public/indexpattern_datasource/loader.ts): https://github.com/elastic/kibana/pull/80132/files#diff-eff297f41b4d66e06e5f7c181f338648e9521022c6945b734cb06b37b2c39bc3R109

dej611 · 2020-10-22T13:48:42Z

I'm working to add more tests on that side to detect changes of this kind in the future 👍

dej611 · 2020-10-22T15:43:00Z

This is the only thing I'm seeing in Lens (installed the logs sample data, no other index patterns around):

It seems like the logic we put in place to remap the field formats to a serialized format don't always work and need a safeguard (x-pack/plugins/lens/public/indexpattern_datasource/loader.ts): #80132 (files)

Apparently formatters into indexPatterns can be either instanciated or already serialized. I've added a serialized version of one formatter in the test to always keep it tracked in the future, and also handle both scenarios.

flash1293 · 2020-10-22T16:32:19Z

Played around with this and I think it's working fine. Would really appreciate another look from @wylieconlon though (as I'm only superficially familiar with that part of Lens).

I did notice a weird behavior, but I'm not sure whether it's related to this PR, so it's fine fixing it separately (noting it here to make sure it isn't):

When switching from explicit ranges to auto histogram, the local formatting is reset (in this example it's jumping from bytes to default) - maybe there is a good reason, but it seemed surprising to me:

The code would benefit from moving some logic into the datasource, but as discussed we can handle this separately.

dej611 · 2020-10-22T16:47:51Z

I see your point.
The reset behaviour is for consistency with the decision ( made in #76121 ) to reset all the configuration when moving from one mode to another, but this case I guess it's not strictly related to that.

flash1293 · 2020-10-22T16:59:32Z

I see, let’s discuss this separately as it’s not related to the changes made here

wylieconlon

Instead of leaving a review with a lot of comments, can you check out my PR against this branch? dej611#2

Basically I've made several change that I think are nicer:

Move the format_column function into the datasource
Simplified the formatting arguments
Renamed the formatted arguments to consistently use format to refer to the number formatter and parentFormat to refer to the ranges formatter

dej611 · 2020-10-26T10:04:20Z

Instead of leaving a review with a lot of comments, can you check out my PR against this branch? dej611#2

Basically I've made several change that I think are nicer:

Move the format_column function into the datasource

Simplified the formatting arguments

Renamed the formatted arguments to consistently use format to refer to the number formatter and parentFormat to refer to the ranges formatter

That looks good. I had a check on it and it seems to better encapsulate the logic with streamlined flow for complex format scenarios.

Organize the formatting differently

dej611 · 2020-10-26T10:05:02Z

@elasticmachine merge upstream

flash1293 · 2020-10-27T11:24:37Z

x-pack/plugins/lens/public/indexpattern_datasource/index.ts

@@ -35,6 +36,7 @@ export class IndexPatternDatasource {
    editorFrame.registerDatasource(async () => {
      const { getIndexPatternDatasource, renameColumns } = await import('../async_services');


I like the new setup of making this an implementation detail of the datasource - can we move the formatColumn import into the async_services so it's loaded async?

kibanamachine · 2020-10-27T15:33:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: adc0ff3

Metrics [docs]

async chunks size

id	before	after	diff
`lens`	1.1MB	1.1MB	+5.6KB

page load bundle size

id	before	after	diff
`data`	1.1MB	1.1MB	+485.0B
`lens`	63.5KB	61.1KB	-2.4KB
total			-1.9KB

History

💚 Build #83892 succeeded bcc2639
💚 Build #83873 succeeded 6e24997
💔 Build #83587 failed 45d0933
💚 Build #83390 succeeded 456a80b
💔 Build #83360 failed 7a93683

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

spalger

Looks like the bundle limit increase was removed 👍 I dig it!

flash1293

Tested and works as expected, LGTM 👍

* master: (87 commits) [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81778) [i18n] add get_kibana_translation_paths tests (elastic#81624) [UX] Fix search term reset from url (elastic#81654) [Lens] Improved range formatter (elastic#80132) [Resolver] `SideEffectContext` changes, remove `@testing-library` uses (elastic#81077) [Time to Visualize] Update Library Text with Call to Action (elastic#81667) [docs] Resolving failed Kibana upgrade migrations (elastic#80999) [ftr/menuToggle] provide helper for enhanced menu toggle handling (elastic#81709) [Task Manager] adds basic observability into Task Manager's runtime operations (elastic#77868) Doc changes for stack management and grouped feature privileges (elastic#80486) Added functional test for alerts list filters by status, alert type and action type. Did a code refactoring and splitting for alerts tests. (elastic#81422) [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415) Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)" [Maps] Use default format when proxying EMS-files (elastic#79760) [Discover] Deangularize context.html (elastic#81442) Use the displayName property in default editor (elastic#73311) adds bug label to Bug report for Security Solution template (elastic#81643) [ML] Transforms: Remove index field limitation for custom query. (elastic#81467) [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390) [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681) ...

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>

dej611 added 5 commits September 18, 2020 14:42

✨ New alternative formatter for ranges

54ed36f

✅ Add range formatter test for alternative format

1904551

Merge remote-tracking branch 'upstream/master' into feature/range-alt…

080eb8a

…ernative-formatter

Merge remote-tracking branch 'upstream/master' into feature/range-alt…

c14ff49

…ernative-formatter

💄 Adopt new range format template

004a580

dej611 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Team:AppArch Feature:Lens v7.11.0 labels Oct 12, 2020

dej611 added 9 commits October 14, 2020 12:51

💄 Revisit styling for range popover

5a1224e

Merge remote-tracking branch 'upstream/master' into feature/range-alt…

fb15462

…ernative-formatter

♻️ Rework the logic of the formatter to better handle the range base …

a058a9c

…and custom scenario

🏷️ fix type issue

2db3014

💄 Use the arrow format rather than dash

142cbfd

💄 Restored append and prepend symbols

8bae6d2

💄 Picked better arrow symbol

68b1f46

💄 Add missing compressed attribute

6557b11

✅ Add more tests for range params scenarios

f664fb0

dej611 marked this pull request as ready for review October 16, 2020 10:47

dej611 requested a review from a team October 16, 2020 10:47

dej611 requested review from a team as code owners October 16, 2020 10:47

Merge branch 'master' into feature/range-alternative-formatter

8d9547c

kibanamachine and others added 2 commits October 21, 2020 06:44

Merge branch 'master' into feature/range-alternative-formatter

b6ed4b8

🐛 Fix fieldFormatMap serialization issue

04b391a

dej611 added 2 commits October 22, 2020 16:09

✅ Add fieldformatMap field to test edge cases

7a93683

🐛 Better handling to support already serialized formatters

456a80b

Organize the formatting differently

6241055

wylieconlon reviewed Oct 22, 2020

View reviewed changes

Merge pull request #2 from wylieconlon/ranges

f7fc98b

Organize the formatting differently

kibanamachine and others added 2 commits October 26, 2020 06:05

Merge branch 'master' into feature/range-alternative-formatter

45d0933

🔧 Raise the limit for data plugin by 5kb

6e24997

dej611 requested a review from a team as a code owner October 27, 2020 08:29

Merge branch 'master' into feature/range-alternative-formatter

bcc2639

flash1293 reviewed Oct 27, 2020

View reviewed changes

♻️ Refactor to use async_services

adc0ff3

spalger approved these changes Oct 27, 2020

View reviewed changes

flash1293 approved these changes Oct 27, 2020

View reviewed changes

dej611 merged commit 8f92de8 into elastic:master Oct 27, 2020

dej611 deleted the feature/range-alternative-formatter branch October 27, 2020 17:08

dej611 mentioned this pull request Oct 27, 2020

[7.x] [Lens] Improved range formatter (#80132) #81826

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Improved range formatter #80132

[Lens] Improved range formatter #80132

dej611 commented Oct 12, 2020 •

edited

Loading

dej611 commented Oct 13, 2020

AlonaNadler commented Oct 13, 2020

cchaos commented Oct 13, 2020

elasticmachine commented Oct 16, 2020

dej611 commented Oct 16, 2020

flash1293 commented Oct 22, 2020 •

edited

Loading

dej611 commented Oct 22, 2020

dej611 commented Oct 22, 2020

flash1293 commented Oct 22, 2020 •

edited

Loading

dej611 commented Oct 22, 2020

flash1293 commented Oct 22, 2020

wylieconlon left a comment

dej611 commented Oct 26, 2020

dej611 commented Oct 26, 2020

flash1293 Oct 27, 2020

kibanamachine commented Oct 27, 2020

spalger left a comment

flash1293 left a comment

		@@ -35,6 +36,7 @@ export class IndexPatternDatasource {
		editorFrame.registerDatasource(async () => {
		const { getIndexPatternDatasource, renameColumns } = await import('../async_services');

[Lens] Improved range formatter #80132

[Lens] Improved range formatter #80132

Conversation

dej611 commented Oct 12, 2020 • edited Loading

Summary

Checklist

dej611 commented Oct 13, 2020

AlonaNadler commented Oct 13, 2020

cchaos commented Oct 13, 2020

elasticmachine commented Oct 16, 2020

dej611 commented Oct 16, 2020

flash1293 commented Oct 22, 2020 • edited Loading

dej611 commented Oct 22, 2020

dej611 commented Oct 22, 2020

flash1293 commented Oct 22, 2020 • edited Loading

dej611 commented Oct 22, 2020

flash1293 commented Oct 22, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

dej611 commented Oct 26, 2020

dej611 commented Oct 26, 2020

flash1293 Oct 27, 2020

Choose a reason for hiding this comment

kibanamachine commented Oct 27, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

page load bundle size

History

spalger left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

dej611 commented Oct 12, 2020 •

edited

Loading

flash1293 commented Oct 22, 2020 •

edited

Loading

flash1293 commented Oct 22, 2020 •

edited

Loading