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] tag cloud #157751

Merged
merged 52 commits into from
Jun 8, 2023
Merged

[lens] tag cloud #157751

merged 52 commits into from
Jun 8, 2023

Conversation

nreese
Copy link
Contributor

@nreese nreese commented May 15, 2023

Part of #154307
Closes #95542

PR adds tagcloud visualization to lens

Screen Shot 2023-05-31 at 1 11 00 PM

@nreese
Copy link
Contributor Author

nreese commented May 30, 2023

@elasticmachine merge upstream

@nreese nreese changed the title [lens] expose canvas tagcloud renderer in lens [lens] tag cloud May 31, 2023
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.

Thanx Nathan! Now it looks great! 2 last comments from me

  1. On no results the icon is not well centered and and the suggestion looks weird. We need to add the tagcloud icon there to work like the rest of the suggestions
  2. We want by clicking a term to create a filter. This is already available in agg based and they share the same renderer so I feel will be easy to enable.
image

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested locally, it looks good to me, but I've identified 2 issues:

  1. there is a warning that pops up if there isn't enough room to render all the tags. Unfortunately, this warning is preserved on each data change even if a new configuration allows a full rendering of all tags. (to reproduce: build a tag cloud with agent.keyword and see the warning now, switch the field to something like geo.dest or extension.keyword. All the tags are visible but the warning remains visible)
  2. It seems that only string fields are supported as tags, but if you drag and drop an IP field into the center area you can also use IPs as tags. I think both ways should allow the same type of fields. (to reproduce: with an empty tag cloud configuration drag clientIp or any IP field into the chart area to see it working, but if you try to pick up the same field from the field dropdown in the tag dimension you can't)

I'd like to rise also this (cc @MichaelMarcialis)

I suggest changing the orientation radio group to a dropdown and making the switch part of the form (with the label at the left and the switch to the right).

I'm not sure for the font-size is not super clear what does the range mean, probably a better label here could help
Screenshot 2023-06-06 at 14 36 09

@nreese
Copy link
Contributor Author

nreese commented Jun 6, 2023

On no results the icon is not well centered and and the suggestion looks weird. We need to add the tagcloud icon there to work like the rest of the suggestions

resolved with 83bd394

We want by clicking a term to create a filter. This is already available in agg based and they share the same renderer so I feel will be easy to enable.

resolved with a482b0e

@nreese nreese requested review from stratoula and dej611 June 6, 2023 19:51
@nreese
Copy link
Contributor Author

nreese commented Jun 6, 2023

@markov00

there is a warning that pops up if there isn't enough room to render all the tags. Unfortunately, this warning is preserved on each data change even if a new configuration allows a full rendering of all tags. (to reproduce: build a tag cloud with agent.keyword and see the warning now, switch the field to something like geo.dest or extension.keyword. All the tags are visible but the warning remains visible)

This is an existing issue and exists in legacy agg based tag cloud visualization. Fixing this issue is outside the scope of this PR.

It seems that only string fields are supported as tags, but if you drag and drop an IP field into the center area you can also use IPs as tags. I think both ways should allow the same type of fields. (to reproduce: with an empty tag cloud configuration drag clientIp or any IP field into the chart area to see it working, but if you try to pick up the same field from the field dropdown in the tag dimension you can't)

resolved with ed2619f

I suggest changing the orientation radio group to a dropdown and making the switch part of the form (with the label at the left and the switch to the right). I'm not sure for the font-size is not super clear what does the range mean, probably a better label here could help

resolved with 062d86a

Screen Shot 2023-06-06 at 2 48 16 PM

@nreese nreese requested a review from markov00 June 6, 2023 20:51
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.

Changes LGTM! I think it works fine as a technical preview visualization in Lens. We can always follow up with improvements. Thanx Nathan, great job!

the only thing I would change before merging is making the label switch and the orientation dropdown one size smaller as it is on the rest toolbar panels, for example

image

I am approving for not blocking this PR

@nreese
Copy link
Contributor Author

nreese commented Jun 7, 2023

the only thing I would change before merging is making the label switch and the orientation dropdown one size smaller as it is on the rest toolbar panels

resolved with 535f1f6

@MichaelMarcialis
Copy link
Contributor

I'd like to rise also this (cc @MichaelMarcialis)

I suggest changing the orientation radio group to a dropdown and making the switch part of the form (with the label at the left and the switch to the right).

I'm not sure for the font-size is not super clear what does the range mean, probably a better label here could help Screenshot 2023-06-06 at 14 36 09

Thanks for the ping, @markov00. I agree with all of those points. Looks like @nreese has addressed them as well. Thanks, all!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1369 1370 +1
expressionGauge 86 87 +1
expressionHeatmap 120 121 +1
expressionPartitionVis 94 95 +1
expressionTagcloud 21 72 +51
expressionXY 151 152 +1
lens 884 893 +9
maps 978 979 +1
total +66

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
@kbn/chart-icons 76 78 +2
expressionTagcloud 5 6 +1
total +3

Async chunks

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

id before after diff
expressionTagcloud 7.7KB 15.0KB +7.2KB
lens 1.2MB 1.3MB +7.5KB
total +14.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
expressionTagcloud 0 2 +2

Page load bundle

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

id before after diff
expressionTagcloud 8.8KB 9.6KB +769.0B
lens 34.9KB 35.2KB +284.0B
total +1.0KB
Unknown metric groups

API count

id before after diff
@kbn/chart-icons 76 78 +2
expressionTagcloud 5 6 +1
total +3

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 497 501 +4
total +6

History

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

@nreese nreese merged commit ec62e15 into elastic:main Jun 8, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 8, 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:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Add word cloud support
8 participants