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] Mosaic / mekko vis type #117668

Merged
merged 22 commits into from
Nov 22, 2021
Merged

[Lens] Mosaic / mekko vis type #117668

merged 22 commits into from
Nov 22, 2021

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Nov 5, 2021

Closes: #104223

Summary

Expose mosaic from charts in Lens for all the great reasons in the charts PR: elastic/elastic-charts#1113

Screens

chart:
image

suggestions:
image

validation:
image

@elastic elastic deleted a comment from kibanamachine Nov 8, 2021
@alexwizp alexwizp self-assigned this Nov 8, 2021
@alexwizp alexwizp added backport:skip This commit does not require backporting v8.1.0 Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Nov 8, 2021
@alexwizp alexwizp marked this pull request as ready for review November 8, 2021 16:00
@alexwizp alexwizp requested a review from a team as a code owner November 8, 2021 16:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

@alexwizp alexwizp requested a review from dej611 November 8, 2021 16:00
@flash1293
Copy link
Contributor

@alexwizp @ghudgins As we discussed via chat, one thing we could to do to make it more obvious to the user there need to be two "slicing" dimensions is to create different groups for the two dimensions.

Right now IMHO it looks a bit weird to have a dimension group with a dimension and it still says "Required" in red.

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

I won't approve yet because it will probably have some changes about the dimension groups.

The code looks good and I tested it on Chrome and works fine. There is one thing though that might be to reconsider, not introduced in this PR, but related. When having a date histogram and switching between any pie type charts, I don't think we should remove the date histogram. We should remove it when switching between xy chart -> pie, but not pie -> donut. @ghudgins @MichaelMarcialis WDYT?
Screenshot 2021-11-11 at 10 07 52

@@ -470,6 +470,7 @@ export type VisualizationDimensionGroupConfig = SharedDimensionProps & {
supportsMoreColumns: boolean;
/** If required, a warning will appear if accessors are empty */
required?: boolean;
minDimensions?: number;
Copy link
Contributor

@mbondyra mbondyra Nov 11, 2021

Choose a reason for hiding this comment

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

seeing the comment from Joe, this will probably change? If for some reason it would still be here(for example, we'd decide to change the messaging instead from Required dimension to Required 2 dimensions) , I wonder if it would make sense to combine it with required as requiredMinDimensionCount... Or something shorter 😅 That would require changes for all vises.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree @mbondyra - the stripping of date histogram seems to make the most sense only from XY --> Partition chart type (and not between the partition chart types)

@alexwizp alexwizp marked this pull request as draft November 11, 2021 10:32
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Nov 15, 2021
@MichaelMarcialis
Copy link
Contributor

When having a date histogram and switching between any pie type charts, I don't think we should remove the date histogram. We should remove it when switching between xy chart -> pie, but not pie -> donut. @ghudgins @MichaelMarcialis WDYT?

I agree with your thinking here, @mbondyra. In the case of your second transition example, I imagine the date histogram dimension should remain as a second dimension to the mosaic chart, thus avoiding the error message entirely.

@monfera
Copy link
Contributor

monfera commented Nov 18, 2021

Great work in this PR 🎉 Here's a sortPredicate function example for the first layer, lmk if there's some specific thing that makes it problematic in Kibana:

sortPredicate: ([name1, node1]: ArrayEntry, [name2, node2]: ArrayEntry) => {
  if (name1 < name2) return -1;
  if (name2 < name1) return 1;

  // otherwise, use the increasing value order
  return node1.value - node2.value;
}

or the briefer, identical

sortPredicate: ([name1, node1]: ArrayEntry, [name2, node2]: ArrayEntry) =>
  name1 < name2 ? -1 : name1 > name2 ? 1 : node1.value - node2.value

image

@alexwizp
Copy link
Contributor Author

@ghudgins I've created a separate issue cause I see similar problem with all Partitions charts. #119147

If we want to implement sorting only for Mosaic, I'll do it in this PR, if not, it should be fixed in a separate one. From my understanding at least for Treemap it should work

@flash1293 fyi

@ghudgins
Copy link
Contributor

it might just because of the styling but I more want to sort mosaic's first dimension than I would on the others. sounds good either way (in this issue or otherwise) but I think it'll come up if we don't fix it. all the more reason to not forget that Experimental badge :)

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

XY transitioning

I have some questions about transitions to/from XY -> Mosaic.

Say I have this XY chart defined with Top value of extension.keyword Horizontal categories, with Break down by Top values of clientip:
Screenshot 2021-11-19 at 15 18 27

When switching to Mosaic the two dimensions gets mapped into Group by with the dimension Top values of clientip (previous Break down by) as inner grouping dimension here.

Screenshot 2021-11-19 at 15 22 32

But if I swap the two dimension in XY and do the same transition, Top values of clientip is still set as inner grouping dimension:
Screenshot 2021-11-19 at 15 23 29

Labels inside or outside?

What does it means in Mosaic context Inside and Outside? To be positions look fixed, no?
Screenshot 2021-11-19 at 17 13 43

Left also a couple of minor nitpicks.

Probably the labels is the one more important to address, as for the transition it would be nice to have it fixed but we had already similar issues afaik.

@flash1293
Copy link
Contributor

Not sure whether the xy transition can be easily fixed in the current code structure. As Alexey noticed this is a difficult area to work with at the moment. I would be totally fine with leaving it in the current state as the nesting is easy to change in mosaic.

@dej611
Copy link
Contributor

dej611 commented Nov 19, 2021

No strong opinion on the transition one. The labels one looks more confusing.

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@dej611
Copy link
Contributor

dej611 commented Nov 22, 2021

The label position has been reworked to show now a classic "Show"/"Hide" options. But, to me, this result is not particularly valuable:

Screenshot 2021-11-22 at 11 18 41

Screenshot 2021-11-22 at 11 18 48

It would make more sense, in my opinion, removing completely the Position label input (with labels always enabled) and keep only the Values visibility, which even when hidden still keeps some context on the grouping:

Screenshot 2021-11-22 at 11 21 15

Screenshot 2021-11-22 at 11 19 51

What do you think @alexwizp , @flash1293, @markov00, @ghudgins ?

@flash1293
Copy link
Contributor

No strong opinion on this one, but I think I agree with you here @dej611 - I can't think of any use cases where people would want to hide the outer labels.

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Reviewed locally in Safari with latest changes 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 683 684 +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 962.6KB 965.4KB +2.8KB

Page load bundle

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

id before after diff
lens 44.1KB 44.2KB +9.0B

History

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

cc @alexwizp

@alexwizp alexwizp merged commit 43f7fc0 into elastic:main Nov 22, 2021
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
* [Lens] Mosaic / mekko vis type

Closes: #104223

* some updates

* fix color palette logic

* fix suggestions

* fix JEST

* fix some parts

* update labels

* Fix JEST

* add showExperimentalBadge

* add sorting

* fix toolbar options

* fix Marco suggestion

Co-authored-by: Kibana Machine <[email protected]>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
* [Lens] Mosaic / mekko vis type

Closes: elastic#104223

* some updates

* fix color palette logic

* fix suggestions

* fix JEST

* fix some parts

* update labels

* Fix JEST

* add showExperimentalBadge

* add sorting

* fix toolbar options

* fix Marco suggestion

Co-authored-by: Kibana Machine <[email protected]>
@nreese nreese mentioned this pull request Apr 6, 2023
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] Mosaic / mekko vis type
10 participants