Tagcloud fixes by thomasneirynck · Pull Request #9194 · elastic/kibana

thomasneirynck · 2016-11-23T00:28:37Z

5.1 is going through QA right now. Will require fixes to Tagcloud.

Created this PR to track smaller fixes/code improvements:

Tag cloud tests are incorrect but still pass #9182: false positive tests
Reporting screenshots show blank image

trevan · 2016-11-23T00:35:03Z

I'm not entirely sure this will fix all that I saw in #9182. If I call "setOptions" with twice with two different scales, I believe the resulting sizes will be wrong. The first time setOptions is called, it will layout and cause the sizes in the words objects to be scaled. The second time setOptions is called, it will use the these new sizes in the scaling instead of the old sizes.

thomasneirynck · 2016-11-23T01:41:03Z

@trevan I agree. To be sure, there's no fix yet for #9182. I just opened this to start a thread on all the outstanding issues for tagcloud (I expect a few more to come in, as we're doing qa now). That initial commit now is a small refactor that addressed some small nits I noticed when revisiting.

epixa · 2016-11-23T15:24:34Z

@thomasneirynck Tests on this one failed

thomasneirynck · 2016-11-23T22:09:44Z

@ppisljar feel free to already take a look. I opened this to work on the fall-out during QA.

There's also an issue with Reporting, causing the container to be too small. I'll leave this PR open to address that too.

jbudz · 2016-11-28T14:29:16Z

jenkins, test this

jbudz · 2016-11-28T19:37:32Z

jenkins, test this

jbudz · 2016-11-28T19:38:33Z

src/core_plugins/tagcloud/public/tag_cloud.js

what's this for?

I added the time-out so as not to trigger a refresh synchronously. Those 10-milliseconds are a magic number.

In hindsight, I prefer it over the requestAnimationFrame, because the DOM-updates are managed by d3, and requestAnimationFrame is better used for drawing to a canvas anyway.

is a timeout of 0 an option and adding comment? Mostly to avoid confusion, that we aren't waiting for something that takes >0 and <10ms

Timeout of 0 would be fine as well. It's basically just to unhook it from the callstack. I'll make that change.

Tagcloud was subsceptible to race conditions, potentially yielding stale clouds. - Introduced queue to explicitly keep track of outstanding jobs and avoid clobbering of the state. - Expanded unit tests

thomasneirynck · 2016-11-28T23:24:11Z

src/core_plugins/tagcloud/public/tag_cloud.less

@@ -1,5 +1,5 @@
 .tagcloud-vis {
-  position: relative;
+  position: absolute;


children of flex-boxes don't stretch to full height, unless the position is absolute (
http://stackoverflow.com/questions/15381172/css-flexbox-child-height-100).

This caused the tagcloud in reporting to be empty.

thomasneirynck · 2016-11-28T23:24:49Z

src/core_plugins/tagcloud/public/tag_cloud_vis.js

  return new TemplateVisType({
    name: 'tagcloud',
    title: 'Tag cloud',
+    implementsRenderComplete: true,


required for Reporting to check the render-counter property, instead of using a timeout

Backports PR #9194 **Commit 1:** Improve robustness Tagcloud was subsceptible to race conditions, potentially yielding stale clouds. - Introduced queue to explicitly keep track of outstanding jobs and avoid clobbering of the state. - Expanded unit tests * Original sha: af5d5c6 * Authored by Thomas Neirynck <thomas@elastic.co> on 2016-11-23T00:20:04Z **Commit 2:** ensure tagcloud expands to full height in reporting * Original sha: de95bf1 * Authored by Thomas Neirynck <thomas@elastic.co> on 2016-11-28T23:13:18Z **Commit 3:** remove magic numbers * Original sha: 011474c * Authored by Thomas Neirynck <thomas@elastic.co> on 2016-11-28T23:33:48Z

Backports PR elastic#9194 **Commit 1:** Improve robustness Tagcloud was subsceptible to race conditions, potentially yielding stale clouds. - Introduced queue to explicitly keep track of outstanding jobs and avoid clobbering of the state. - Expanded unit tests * Original sha: af5d5c6 * Authored by Thomas Neirynck <thomas@elastic.co> on 2016-11-23T00:20:04Z **Commit 2:** ensure tagcloud expands to full height in reporting * Original sha: de95bf1 * Authored by Thomas Neirynck <thomas@elastic.co> on 2016-11-28T23:13:18Z **Commit 3:** remove magic numbers * Original sha: 011474c * Authored by Thomas Neirynck <thomas@elastic.co> on 2016-11-28T23:33:48Z Former-commit-id: 67aea7b

- `@elastic/eui`: `v109.0.0` ⏩ `v109.1.0` - `@elastic/eslint-plugin-eui`: `v2.5.0` ⏩ `v2.6.0` --- ## Changes - Updated i18n EUI mapping 6cc95b0 - Updated test in Unified Search 668948f ## Package updates ### `@elastic/eui` [`v109.1.0`](https://github.com/elastic/eui/releases/v109.1.0) - Added `--euiBottomBarOffset` CSS variable to `EuiBottomBar` for positioning other fixed elements relative to the bottom bar's height ([#9211](elastic/eui#9211)) - Updated `boxesVertical` icon and restored `checkInCircleFilled`, `errorFilled`, and `warningFilled` icons. ([#9194](elastic/eui#9194)) - Updated `EuiSuperDatePicker` with new time zone information, opt-in via `timeZoneDisplayProps`. ([#9191](elastic/eui#9191)) - Updated the position of `EuiModal` by removing bottom padding in `EuiOverlayMask` ([#9190](elastic/eui#9190)) - Added `EuiPopover` and `EuiToolTip`'s `repositionOnScroll` to `componentDefaults` ([#9152](elastic/eui#9152)) - Updated `EuiSuperDatePicker` with new time window buttons for time shifting and zoom out, opt-in via `showTimeWindowButtons` boolean prop. ([#9151](elastic/eui#9151)) - Added beta prop `hasAriaDisabled` to all base button components: `EuiButton`, `EuiButtonEmpty`, `EuiButtonIcon`, `EuibuttonGroup`, `EuiFilterButton` ([#9201](elastic/eui#9201)) - Added `euiDisabledSelector` variable that combines CSS selectors `:disabled` and `[aria-disabled="true"]` ([#9201](elastic/eui#9201)) - Added custom test matchers that check for both `disabled` and `aria-disabled` attributes: ([#9201](elastic/eui#9201)) - React testing Library: `.toBeEuiDisabled()` - Enzyme: `.toHaveEuiDisabledProp()` - Cypress: `should('be.euiDisabled)` **Bug fixes** - Fixed unexpected duplicate columns in `EuiDataGrid` crashing the column sorting by removing duplicate columns entirely ([#9209](elastic/eui#9209)) - Fixed a visual bug in `EuiTable` where long table row content would be cut off on mobile screens ([#9206](elastic/eui#9206)) - Fixed virtualized `EuiCodeBlock` rendering blank lines when content updates if scrolled. ([#9196](elastic/eui#9196)) - Fixed `EuiButtonGroup` button sizing to ensure square buttons when used with `isIconOnly=true` ([#9170](elastic/eui#9170)) **Accessibility** - Fixed an issue where portalled components like `EuiPopover` were not included in `EuiFlyout`'s focus trap through `includeSelectorInFocusTrap`, making them inaccessible to keyboard users ([#9103](elastic/eui#9103)) ### `@elastic/eslint-plugin-eui` [`v2.6.0`](https://github.com/elastic/eui/blob/main/packages/eslint-plugin/changelogs/CHANGELOG_2025.md#v260) - Added new `require-table-caption` rule. ([#9168](elastic/eui#9168)) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

- `@elastic/eui`: `v109.0.0` ⏩ `v109.1.0` - `@elastic/eslint-plugin-eui`: `v2.5.0` ⏩ `v2.6.0` --- ## Changes - Updated i18n EUI mapping 6cc95b0 - Updated test in Unified Search 668948f ## Package updates ### `@elastic/eui` [`v109.1.0`](https://github.com/elastic/eui/releases/v109.1.0) - Added `--euiBottomBarOffset` CSS variable to `EuiBottomBar` for positioning other fixed elements relative to the bottom bar's height ([elastic#9211](elastic/eui#9211)) - Updated `boxesVertical` icon and restored `checkInCircleFilled`, `errorFilled`, and `warningFilled` icons. ([elastic#9194](elastic/eui#9194)) - Updated `EuiSuperDatePicker` with new time zone information, opt-in via `timeZoneDisplayProps`. ([elastic#9191](elastic/eui#9191)) - Updated the position of `EuiModal` by removing bottom padding in `EuiOverlayMask` ([elastic#9190](elastic/eui#9190)) - Added `EuiPopover` and `EuiToolTip`'s `repositionOnScroll` to `componentDefaults` ([elastic#9152](elastic/eui#9152)) - Updated `EuiSuperDatePicker` with new time window buttons for time shifting and zoom out, opt-in via `showTimeWindowButtons` boolean prop. ([elastic#9151](elastic/eui#9151)) - Added beta prop `hasAriaDisabled` to all base button components: `EuiButton`, `EuiButtonEmpty`, `EuiButtonIcon`, `EuibuttonGroup`, `EuiFilterButton` ([elastic#9201](elastic/eui#9201)) - Added `euiDisabledSelector` variable that combines CSS selectors `:disabled` and `[aria-disabled="true"]` ([elastic#9201](elastic/eui#9201)) - Added custom test matchers that check for both `disabled` and `aria-disabled` attributes: ([elastic#9201](elastic/eui#9201)) - React testing Library: `.toBeEuiDisabled()` - Enzyme: `.toHaveEuiDisabledProp()` - Cypress: `should('be.euiDisabled)` **Bug fixes** - Fixed unexpected duplicate columns in `EuiDataGrid` crashing the column sorting by removing duplicate columns entirely ([elastic#9209](elastic/eui#9209)) - Fixed a visual bug in `EuiTable` where long table row content would be cut off on mobile screens ([elastic#9206](elastic/eui#9206)) - Fixed virtualized `EuiCodeBlock` rendering blank lines when content updates if scrolled. ([elastic#9196](elastic/eui#9196)) - Fixed `EuiButtonGroup` button sizing to ensure square buttons when used with `isIconOnly=true` ([elastic#9170](elastic/eui#9170)) **Accessibility** - Fixed an issue where portalled components like `EuiPopover` were not included in `EuiFlyout`'s focus trap through `includeSelectorInFocusTrap`, making them inaccessible to keyboard users ([elastic#9103](elastic/eui#9103)) ### `@elastic/eslint-plugin-eui` [`v2.6.0`](https://github.com/elastic/eui/blob/main/packages/eslint-plugin/changelogs/CHANGELOG_2025.md#v260) - Added new `require-table-caption` rule. ([elastic#9168](elastic/eui#9168)) --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v5.1.0 labels Nov 23, 2016

ppisljar self-assigned this Nov 23, 2016

thomasneirynck force-pushed the tagcloud-fix branch from aebd62c to fb323b9 Compare November 23, 2016 21:59

jbudz self-assigned this Nov 28, 2016

thomasneirynck mentioned this pull request Nov 28, 2016

[5.x] Tagcloud #9205

Merged

jbudz reviewed Nov 28, 2016

View reviewed changes

thomasneirynck added 2 commits November 28, 2016 18:22

Improve robustness

af5d5c6

Tagcloud was subsceptible to race conditions, potentially yielding stale clouds. - Introduced queue to explicitly keep track of outstanding jobs and avoid clobbering of the state. - Expanded unit tests

ensure tagcloud expands to full height in reporting

de95bf1

thomasneirynck force-pushed the tagcloud-fix branch from 3133c63 to de95bf1 Compare November 28, 2016 23:22

thomasneirynck commented Nov 28, 2016

View reviewed changes

remove magic numbers

011474c

thomasneirynck added the v6.0.0 label Nov 28, 2016

jbudz approved these changes Nov 29, 2016

View reviewed changes

epixa merged commit 8a528dc into elastic:master Nov 29, 2016

This was referenced Nov 29, 2016

[5.x] Tagcloud fixes #9246

Merged

[5.1] Tagcloud fixes #9247

Merged

epixa added the v5.2.0 label Nov 29, 2016

ppisljar added the backported label Dec 2, 2016

epixa added v5.1.1 and removed v5.1.1 v5.1.0 labels Dec 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tagcloud fixes#9194

Tagcloud fixes#9194
epixa merged 3 commits intoelastic:masterfrom
thomasneirynck:tagcloud-fix

thomasneirynck commented Nov 23, 2016 •

edited by epixa

Loading

Uh oh!

trevan commented Nov 23, 2016

Uh oh!

thomasneirynck commented Nov 23, 2016

Uh oh!

epixa commented Nov 23, 2016

Uh oh!

thomasneirynck commented Nov 23, 2016

Uh oh!

jbudz commented Nov 28, 2016

Uh oh!

jbudz commented Nov 28, 2016

Uh oh!

jbudz Nov 28, 2016

Uh oh!

thomasneirynck Nov 28, 2016

Uh oh!

jbudz Nov 28, 2016 •

edited

Loading

Uh oh!

thomasneirynck Nov 28, 2016

Uh oh!

thomasneirynck Nov 28, 2016

Uh oh!

thomasneirynck Nov 28, 2016 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

thomasneirynck commented Nov 23, 2016 • edited by epixa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trevan commented Nov 23, 2016

Uh oh!

thomasneirynck commented Nov 23, 2016

Uh oh!

epixa commented Nov 23, 2016

Uh oh!

thomasneirynck commented Nov 23, 2016

Uh oh!

jbudz commented Nov 28, 2016

Uh oh!

jbudz commented Nov 28, 2016

Uh oh!

jbudz Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

jbudz Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thomasneirynck commented Nov 23, 2016 •

edited by epixa

Loading

jbudz Nov 28, 2016 •

edited

Loading

thomasneirynck Nov 28, 2016 •

edited

Loading