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.

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

[Lens] Add Multi terms support to Top Values #118600

Merged
merged 64 commits into from
Dec 8, 2021
Merged

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Nov 15, 2021

Summary

Fix #95079

This PR introduces an interface for Lens of the Multi terms value aggregation, via the Top Values function.
This PR does not contain the Combine drag and drop action. Will be added in a follow up PR.

When a new term field is added the current fields used are removed from the field dropdown list.
When more than one field is defined scripted fields are filtered from the field dropdown list, and where already set are marked as invalid with an explanation (both in the workspace and in the panel) on how to fix it.

Screenshot 2021-11-25 at 15 14 32

Screenshot 2021-11-25 at 15 14 44

Screenshot 2021-11-25 at 15 15 24

Screenshot 2021-11-25 at 15 15 39

Time shift conflict is resolved similarly for the single term, concatenating conditions of multiple terms:

Screenshot 2021-11-25 at 15 16 00

Screenshot 2021-11-25 at 15 16 14

Screenshot 2021-11-25 at 15 16 31

Terms can be reordered manually via Drag and Drop:

reorder_terms

The empty field can be reordered as well but it has no effect until it contains a value. Closing and reopening the panel when empty will just discard it:

reorder_empty_terms

In case of 2 fields, where only one is filled with a value, the trash icon will be enabled only for the empty one:
Screenshot 2021-11-25 at 15 21 28

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Nov 15, 2021
@ghudgins
Copy link
Contributor

ghudgins commented Nov 16, 2021

Will list things here as I find them:

  • couldn't filter when left clicking multi terms in a stacked bar, it just filters time. Expected the "Select filters to apply" modal with checkbox choices
  • I wish legend truncation was better automatically - all multi fields are going to likely run into the 1 line fast

@dej611
Copy link
Contributor Author

dej611 commented Nov 19, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@mbondyra
Copy link
Contributor

mbondyra commented Dec 2, 2021

The issues I mentioned before are all fixed. One small thing to correct (here or in the next PR if you prefer to:)

  • dropping a filed to 2-fields multifield replaces the first field. Not sure if that's desired behavior. Maybe instead we should replace the whole dimension with a new top values?
one.mp4
  • when switching between top values and filters we make a default filter with the field from top values. For multi values, we only do it for the first field. Maybe it's worth to expand it to all fields?
two.mp4

@dej611
Copy link
Contributor Author

dej611 commented Dec 2, 2021

  • dropping a filed to 2-fields multifield replaces the first field. Not sure if that's desired behavior. Maybe instead we should replace the whole dimension with a new top values?

The drag and drop behaviour has not be considered in this PR yet, but I have prepared a follow up PR which adds a combine action on drop, plus other drop handlers: #119841
I'll make sure to add a test case on that branch for this replace scenario.

  • when switching between top values and filters we make a default filter with the field from top values. For multi values, we only do it for the first field. Maybe it's worth to expand it to all fields?

This is definitely a bug in this PR. Will address it.

@dej611
Copy link
Contributor Author

dej611 commented Dec 2, 2021

I'd leave the option and keep > as default (note whitespaces around it)

@Dosant I've updated the default char in the formatter.
Right now I've just replaced the single char: do you prefer having a full string replacement instead?

const separator = params.separator ?? ' > '
  • dropping a filed to 2-fields multifield replaces the first field. Not sure if that's desired behavior. Maybe instead we should replace the whole dimension with a new top values?

I've tested this behaviour also in th linked PR and it was a full bug. Fixed the behaviour plus one more unit test for it. 👍

when switching between top values and filters we make a default filter with the field from top values. For multi values, we only do it for the first field. Maybe it's worth to expand it to all fields?

Also addressed this, adding tests on the filters side.

@Dosant
Copy link
Contributor

Dosant commented Dec 2, 2021

I'd leave the option and keep > as default (note whitespaces around it)

@Dosant I've updated the default char in the formatter. Right now I've just replaced the single char: do you prefer having a full string replacement instead?

const separator = params.separator ?? ' > '
const separator = params.separator ?? ' > '

is more flexible, so I'd keep as it was :D

@dej611
Copy link
Contributor Author

dej611 commented Dec 2, 2021

@Dosant restored the previous version + added a couple of unit tests.

@mbondyra mbondyra self-requested a review December 3, 2021 13:43
@stratoula
Copy link
Contributor

@dej611 while the legend is not truncated for xy charts
image

for the partition charts is truncated. Is this intended?
image

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

I am approving as I have only found this minor issue. Feel free to merge when this is solved! What a lovely PR 👏

@dej611
Copy link
Contributor Author

dej611 commented Dec 3, 2021

@stratoula refactored the truncation logic into a shared helper and added few tests for it.
Now XY, Pie and Heatmap charts use the shared function for truncation default.

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@mbondyra
Copy link
Contributor

mbondyra commented Dec 8, 2021

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 727 732 +5

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2904 2905 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.1MB 1.1MB +8.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 442.9KB 443.3KB +338.0B
Unknown metric groups

API count

id before after diff
data 3318 3319 +1

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Allow multi field "top values"
9 participants