Skip to content

Conversation

@sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Nov 23, 2020

Summary

Fixes #83876

The regression was caused by #77910,
more detailed explanation of the regression described here

Explicitly pass each tagcloud expression param to avoid passing extra one from params of saved object.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@sulemanof
Copy link
Contributor Author

sulemanof commented Nov 23, 2020

@lukeelmers @ppisljar would be great if someone of you could give more detailed explanation of why a common set of visConfig was replaced to actual arguments, which should be registered in a function definition? (#33773)
That means if we need to add any simple param as an option into the visualization, we should also specify each as additional arg in function definition.
And why the only tagcloud function definition was changed in this way, but not others like pie_fn, vis_type_vislib_vis_fn and so on ?

@ppisljar
Copy link
Contributor

in order to make functions more semantic/usable we want all the arguments there explicitly. Typing json in as argument is not very nice (think about canvas).

we only converted metric and tagcloud as example, and all the others will be converted by App team

@sulemanof sulemanof added Feature:Tagcloud Tag cloud visualization feature release_note:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.10.1 v7.11.0 v8.0.0 labels Nov 23, 2020
@sulemanof sulemanof marked this pull request as ready for review November 23, 2020 17:35
@sulemanof sulemanof requested a review from a team November 23, 2020 17:35
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

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

id before after diff
visTypeTagcloud 19.6KB 18.8KB -734.0B

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

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Code review only; but updates LGTM

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.

LGTM, tested it locally on Safari

@sulemanof sulemanof merged commit 91ff8e4 into elastic:master Nov 24, 2020
sulemanof pushed a commit to sulemanof/kibana that referenced this pull request Nov 24, 2020
sulemanof pushed a commit to sulemanof/kibana that referenced this pull request Nov 24, 2020
sulemanof pushed a commit that referenced this pull request Nov 24, 2020
sulemanof pushed a commit that referenced this pull request Nov 24, 2020
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:fix Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.10.1 v7.11.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrated tag cloud from 5.6.16 displays error in 7.10/7.11

6 participants