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

Fix tagcloud word overlapping #81161

Merged
merged 3 commits into from
Oct 21, 2020
Merged

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Oct 20, 2020

Summary

Fixes #78407

This fixes word overlapping in tagcloud visualization when it is resized.
The fix returns back the next code, which was omitted in a recent PR - seems it is responsible for additional layout calculations and still needed before resize:

  this._updateParams(visParams);
  this._updateData(data);

This also changes the fontFamily to Inter UI.
Can't skip setting the fontFamily explicitly (make it inherit), since the setting is necessary for calculating tagcloud layout and word spacing.

The final result:

tagcloud_fix

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@sulemanof sulemanof added Feature:Tagcloud Tag cloud visualization feature Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 20, 2020
@sulemanof sulemanof marked this pull request as ready for review October 20, 2020 15:13
@sulemanof sulemanof requested a review from a team October 20, 2020 15:13
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
visTypeTagcloud 295.2KB 295.2KB +15.0B

History

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

* the fontFamily should be set explicitly for calculating a layout
* and to avoid words overlapping
*/
this._fontFamily = 'Inter UI, sans-serif';
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we can't avoid this, is a known issue on d3-tagcloud

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.

Code LGTM, tested it on FF and Chrome and works as it should. 👏

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally.

@sulemanof sulemanof merged commit e8fffe9 into elastic:master Oct 21, 2020
@sulemanof sulemanof deleted the fix/tagcloud_overlap branch October 21, 2020 10:32
sulemanof added a commit to sulemanof/kibana that referenced this pull request Oct 21, 2020
* Fix overlaps on resize

* Set up font family

* Update snapshots
sulemanof added a commit to sulemanof/kibana that referenced this pull request Oct 21, 2020
* Fix overlaps on resize

* Set up font family

* Update snapshots
sulemanof added a commit that referenced this pull request Oct 21, 2020
* Fix overlaps on resize

* Set up font family

* Update snapshots
sulemanof added a commit that referenced this pull request Oct 21, 2020
* Fix overlaps on resize

* Set up font family

* Update snapshots
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 21, 2020
…arm-phase-to-formlib

* 'master' of github.com:elastic/kibana: (55 commits)
  [UX] Fix map color variance and apply proper filter for extended stats (elastic#81106)
  [User Experience] Use EuiSelect for percentiles instead of SuperSelect (elastic#81082)
  [DOCS] Add link for monitoring ssl settings (elastic#81057)
  [test] Await loading indicator in monitoring test (elastic#81279)
  [ILM] Minor copy and link additions to cloud CTA for cold phase (elastic#80512)
  [Mappings editor] Add scaled_float and date_range comp integration tests (elastic#81287)
  [Discover] Deangularize context.app (elastic#80851)
  [O11y Overview] Add code to display/hide UX section when appropriate (elastic#80873)
  [Discover] Extend DiscoverNoResults component to show different message on error (elastic#79671)
  Fix tagcloud word overlapping (elastic#81161)
  [Security Solution] Fixes flaky test rules (elastic#81040)
  Changed the code to avoid tech debt with hacky solutions after receiving comments on EUI issue reported about this problem. (elastic#81183)
  [Security Solution][All] Replace old markdown renderer with the new one (elastic#80301)
  Add namespaced version of the API call (elastic#81278)
  [ML] Data Frame Analytics: Fix race condition and support for feature influence legacy format. (elastic#81123)
  [Fleet] Fix POLICY_CHANGE action creation for new policy (elastic#81236)
  [Security Solution][Endpoint][Admin] Malware user notification checkbox (elastic#78084)
  [SecuritySolution][Unit Tests] - fix flakey unit test (elastic#81239)
  skip flaky suite (elastic#81264)
  [Maps] fix top-level Map page is called 'Kibana' (elastic#81238)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/forcemerge_field.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Tagcloud Tag cloud visualization feature release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make tagcloud vis to use kibana font family
5 participants