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

feat(wordcloud): wordcloud #1038

Merged
merged 21 commits into from
Mar 22, 2021
Merged

feat(wordcloud): wordcloud #1038

merged 21 commits into from
Mar 22, 2021

Conversation

Kati-dev-hu
Copy link
Contributor

@Kati-dev-hu Kati-dev-hu commented Feb 28, 2021

Summary

Initial wordcloud implementation

image
image
image

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 28, 2021

💚 CLA has been signed

@monfera monfera added the wip work in progress label Feb 28, 2021
@monfera monfera marked this pull request as draft February 28, 2021 18:33
@markov00
Copy link
Member

markov00 commented Mar 5, 2021

Hi @Kati-dev-hu thanks so much for that, we really appreciate it!
I've played a bit with the examples and I've to say that it looks amazing!
I've compared it to the Kibana implementation and looks like we have nearly all required features. What is missing is the following:

  • the user can select the type of scale applied to each text weight: linear, log and square root are the options here.
  • There are some specific cases where, due to the applied constraints like the number of tags, font size, etc, some words are filtered out (you can try that just by reducing the width of the chart to a very small size). Kibana notify the user about that with a small icon over imposed on the chart with a tooltip

Screenshot 2021-03-05 at 11 39 15

There can be several ways to achieve this, for now, it also fine living without it, but it will be great at least to have an internal logic that checks if all the words will be rendered or not. We can then use that somehow to expose that information to Kibana/other consumers

Anyway I think this already works very well and looks very clean, thanks for this big help!

@Kati-dev-hu
Copy link
Contributor Author

Hi @markov00, thanks for your comments. Text weight is resolved, and the logic for missing words is coming up too, pushing commits soon

image

@Kati-dev-hu
Copy link
Contributor Author

Commit pushed:
1/ warning in case not all words can be placed,
2/ text weight amongst knobs (weightFun):

image

@Kati-dev-hu Kati-dev-hu force-pushed the wordcloud-wip branch 2 times, most recently from 61d147b to a1e9f2c Compare March 21, 2021 19:30
@codecov-io
Copy link

codecov-io commented Mar 21, 2021

Codecov Report

Merging #1038 (a0f5965) into master (588bdfa) will decrease coverage by 0.51%.
The diff coverage is 39.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
- Coverage   72.72%   72.20%   -0.52%     
==========================================
  Files         367      397      +30     
  Lines       11412    12058     +646     
  Branches     2479     2560      +81     
==========================================
+ Hits         8299     8707     +408     
- Misses       3098     3328     +230     
- Partials       15       23       +8     
Flag Coverage Δ
unittests 71.77% <39.27%> (-0.95%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/chart_types/index.ts 100.00% <ø> (ø)
...hart_types/wordcloud/layout/viewmodel/viewmodel.ts 8.33% <8.33%> (ø)
...pes/wordcloud/renderer/svg/connected_component.tsx 19.16% <19.16%> (ø)
...t_types/wordcloud/state/selectors/picked_shapes.ts 20.00% <20.00%> (ø)
...hart_types/wordcloud/state/selectors/scenegraph.ts 35.71% <35.71%> (ø)
...c/chart_types/wordcloud/state/selectors/tooltip.ts 38.46% <38.46%> (ø)
src/state/chart_state.ts 87.17% <50.00%> (-0.98%) ⬇️
...wordcloud/state/selectors/on_element_out_caller.ts 53.33% <53.33%> (ø)
...ordcloud/state/selectors/on_element_over_caller.ts 53.33% <53.33%> (ø)
...rdcloud/state/selectors/on_element_click_caller.ts 56.25% <56.25%> (ø)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 588bdfa...a0f5965. Read the comment docs.

@monfera monfera marked this pull request as ready for review March 21, 2021 21:27
@monfera monfera changed the title feat(wordcloud): wordcloud WIP feat(wordcloud): wordcloud Mar 21, 2021
@monfera monfera removed the wip work in progress label Mar 21, 2021
@monfera
Copy link
Contributor

monfera commented Mar 21, 2021

jenkins test this please

@monfera
Copy link
Contributor

monfera commented Mar 21, 2021

jenkins test this please

@monfera monfera requested a review from markov00 March 22, 2021 07:52
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.

Everything looks great, I've added few comments that we can easily fix on a subsequent PR.
Thank you very much for this great contribution!

});

/** @public */
export type WeightFun = Values<typeof WeightFun>;
Copy link
Member

Choose a reason for hiding this comment

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

nit: Function is usually abbreviated as Fn

Comment on lines +140 to +142
outOfRoomCallback: (wordCount, renderedWordCount) => {
Logger.warn(`Not all words have been placed: ${renderedWordCount} words rendered out of ${wordCount}`);
},
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure about having that console log by default. We can probably have a no-op here

Comment on lines +281 to +295
private tryCanvasContext() {
const canvas = this.props.forwardStageRef.current;
this.ctx = canvas && canvas.getContext('2d');
}

private drawCanvas() {
if (this.ctx) {
/* const { width, height }: Dimensions = this.props.chartContainerDimensions;
renderCanvas2d(this.ctx, this.devicePixelRatio, {
...this.props.geometries,
config: { ...this.props.geometries.config, width, height },
});
*/
}
}
Copy link
Member

Choose a reason for hiding this comment

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

cam we completely remove the canvas element here and on the rendering? If we are not using it right now it's better to clean this up

}

getTooltipInfo(globalState: GlobalChartState) {
return getTooltipInfoSelector(globalState);
Copy link
Member

Choose a reason for hiding this comment

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

The tooltip doesn't bring any new info as it is proposed today. We can easily disable it

@monfera monfera merged commit f08f4c9 into elastic:master Mar 22, 2021
@Kati-dev-hu Kati-dev-hu mentioned this pull request Mar 22, 2021
4 tasks
@Kati-dev-hu
Copy link
Contributor Author

Kati-dev-hu commented Mar 22, 2021

Hi @markov00 Thanks for the review, I created the follow-up issue with these steps

github-actions bot pushed a commit that referenced this pull request Mar 23, 2021
# [25.4.0](v25.3.0...v25.4.0) (2021-03-23)

### Bug Fixes

* chromium area path render bug ([#1067](#1067)) ([e16d15d](e16d15d))

### Features

* **tooltip:** expose datum in the TooltipValue ([#1082](#1082)) ([0246784](0246784)), closes [#1042](#1042)
* **wordcloud:** wordcloud ([#1038](#1038)) ([f08f4c9](f08f4c9))
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 25.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants