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

Jump to bottom

[Tagcloud] Replaces current implementation with elastic-charts #100017

Merged
merged 23 commits into from Jun 15, 2021
Merged

[Tagcloud] Replaces current implementation with elastic-charts #100017

merged 23 commits into from Jun 15, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented May 13, 2021 •
edited

Summary

Closes #95539 Closes #101021 Closes #94043 Closes #93665 Closes #93664 Closes #89523 Closes #22382

Implementation of tagcloud with es-charts

image

Done in this PR

  • Remove d3-cloud from kibana
  • Replace the current implementation with es-charts wordcloud
  • Use the new palette service
  • Colors are synced on a dashboard
  • Remove all the js used in vis_type_tagcloud

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula changed the title Replace tagcloud es charts [Tagcloud] Replaces current implementation with es-charts May 14, 2021
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula mentioned this pull request Jun 3, 2021
Closed
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibanamachine Jun 4, 2021
@stratoula stratoula added Feature:Tagcloud Tag cloud visualization feature v7.14.0 v8.0.0 release_note:feature Makes this part of the condensed release notes labels Jun 9, 2021
@stratoula stratoula marked this pull request as ready for review June 9, 2021 14:55
@stratoula stratoula requested a review from a team June 9, 2021 14:55
@stratoula stratoula requested review from a team as code owners June 9, 2021 14:55
@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jun 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula stratoula requested a review from markov00 June 9, 2021 14:56
pgayvallet
pgayvallet approved these changes Jun 9, 2021
View reviewed changes
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

home changes lgtm

@elastic elastic deleted a comment from kibanamachine Jun 14, 2021
flash1293
flash1293 requested changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

The color palette assigns the "first" color to the highest value and the last color to the lowest value
Screenshot 2021-06-14 at 13 26 01
Screenshot 2021-06-14 at 13 26 31

While that's not necessarily wrong, it's not what I would have expected - should we turn it around?

Found one important bug: Tags are not clickable on a dashboard

@stratoula
Copy link
Contributor Author

stratoula commented Jun 14, 2021 •
edited

@flash1293 I can't replicate it. Click event works for me both on the editor and on the dashboard.

About the coloring, I thought about it, but I decided to follow the old implementation. I am open to discussion though. I have no strong feelings about it :D I preferred that way only because I don't want to introduce changes to our users that use the compatibility palette.

@flash1293
Copy link
Contributor

@stratoula I will check with a clean checkout again, but it always happens for me, for really basic charts:
Screenshot 2021-06-14 at 15 58 54

Won't get a pointer cursor and it doesn't do anything on click

@stratoula
Copy link
Contributor Author

stratoula commented Jun 14, 2021 •
edited

@flash1293 this was a late addition on the es-charts so maybe you need a cleanup on the library. About the cursor pointer, thanks for reminding me of this :D @markov00 I think it will be good to change the cursor on each tag, to visualize that it is clickable.
Not sure if this is a blocker for this PR though.

flash1293
flash1293 approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Pulling in the latest version made clicking work, but the cursor should be fixed.

I see what you mean about coloring - LGTM

@ghudgins ghudgins changed the title [Tagcloud] Replaces current implementation with es-charts [Tagcloud] Replaces current implementation with elastic-charts Jun 14, 2021
@stratoula
Copy link
Contributor Author

stratoula commented Jun 14, 2021 •
edited

I will solve it with css in this PR

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTagcloud 30 24 -6

Async chunks

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

id before after diff
visTypeTagcloud 268.6KB 17.8KB -250.8KB

Page load bundle

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

id before after diff
visTypeTagcloud 15.6KB 14.4KB -1.2KB
Unknown metric groups

async chunk count

id before after diff
visTypeTagcloud 1 2 +1

History

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

@stratoula stratoula merged commit 6351d51 into elastic:master Jun 15, 2021
stratoula added a commit to stratoula/kibana that referenced this pull request Jun 15, 2021
@stratoula @kibanamachine
[Tagcloud] Replaces current implementation with elastic-charts (elast…
c0d743f 
…ic#100017)

* WIP - Replace tagcloud with es-charts wordcloud

* Cleanup and add unit tests

* Fix interpreter test

* Update all tagcloud snapshots

* Partial fix tagcloud test

* Fix some other functional tests, add migration script, update sample data

* Replace getColor with getCategorixalColor

* Fix functional test

* Apply clickhandler event for filtering by clicking the word

* Fix weight calculation

* Add a unit test and fix functional

* Change the cursor to pointer

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@stratoula stratoula mentioned this pull request Jun 15, 2021
Merged
stratoula added a commit that referenced this pull request Jun 15, 2021
@stratoula @kibanamachine
[Tagcloud] Replaces current implementation with elastic-charts (#100017…
815c3ce 
…) (#102150)

* WIP - Replace tagcloud with es-charts wordcloud

* Cleanup and add unit tests

* Fix interpreter test

* Update all tagcloud snapshots

* Partial fix tagcloud test

* Fix some other functional tests, add migration script, update sample data

* Replace getColor with getCategorixalColor

* Fix functional test

* Apply clickhandler event for filtering by clicking the word

* Fix weight calculation

* Add a unit test and fix functional

* Change the cursor to pointer

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
cuff-links pushed a commit to cuff-links/kibana that referenced this pull request Jun 15, 2021
@stratoula @kibanamachine
[Tagcloud] Replaces current implementation with elastic-charts (elast…
7e41ee1 
…ic#100017)

* WIP - Replace tagcloud with es-charts wordcloud

* Cleanup and add unit tests

* Fix interpreter test

* Update all tagcloud snapshots

* Partial fix tagcloud test

* Fix some other functional tests, add migration script, update sample data

* Replace getColor with getCategorixalColor

* Fix functional test

* Apply clickhandler event for filtering by clicking the word

* Fix weight calculation

* Add a unit test and fix functional

* Change the cursor to pointer

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankeairns ryankeairns ryankeairns approved these changes

@flash1293 flash1293 flash1293 approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@markov00 markov00 Awaiting requested review from markov00

Assignees
No one assigned
Labels
Feature:Tagcloud Tag cloud visualization feature release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failing test: Jest Tests.src/plugins/vis_type_tagcloud/public/components - tag cloud tests should only send single renderComplete event at the very end, using 500ms timeout completeness should be ok Failing test: Chrome UI Functional Tests.test/functional/apps/visualize/_tag_cloud·ts - visualize app tag cloud chart formatted field "after all" hook for "should apply filter with unformatted value" Failing test: Chrome UI Functional Tests.test/functional/apps/visualize/_tag_cloud·ts - visualize app tag cloud chart formatted field "before all" hook for "should format tags with field formatter" Failing test: Jest Tests.src/plugins/vis_type_tagcloud/public/components - tag cloud tests should only send single renderComplete event at the very end, using 300ms timeout completeness should be ok Tag Cloud in a dashboard isn't setting data-render-complete="true"
[TagCloud] Replace the current implementation with the elastic-charts one [Reporting] Blank reports for tag clouds visualizations
6 participants
@stratoula @elasticmachine @flash1293 @kibanamachine @ryankeairns @pgayvallet