-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Canvas] TagCloud
arguments.
#107729
[Canvas] TagCloud
arguments.
#107729
Conversation
TagCloud
arguments.
Pinging @elastic/kibana-presentation (Team:Presentation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
storybook aliases LGTM
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presentation changes look good.
onValueChange, | ||
argValue, | ||
argId, | ||
choises: dynamicChoises, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Spelling is choices
dynamiceChoices
. Needs to be corrected in a few places.
); | ||
|
||
const options = [ | ||
{ value: '', text: 'select column', disabled: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static text Select Column
should be an i18n string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
argId, | ||
choises: dynamicChoises, | ||
}) => { | ||
const existChoises = typeInstance.options.choices ?? dynamicChoises?.[typeInstance.name] ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain where the dynamic choices option comes from? I'm not sure I'm following why it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it seems to me that is from the version with expression builder. I'll remove it. Before, it was allowing to pass some options to select
from the resolve
method of the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely removed the current part of the code.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elasticmachine merge upstream |
02f4c87
to
bf3f3c8
Compare
@stratoula, I've fixed the code to avoid migrations. |
@elasticmachine merge upstream |
setPalette(paramName, { | ||
type: 'palette', | ||
name: newPalette, | ||
name: palette?.value ?? 'clear', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the fallback to clear does here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stratoula, I've updated the code.
This line is updating the color of the chart for the default palette if active palette doesn't exist in a palettes list:
kibana/src/plugins/vis_default_editor/public/components/controls/palette_picker.tsx
Line 61 in 18514ce
name: palette?.value ?? DEFAULT_PALETTE, |
This line is changing the palette in palette picker to default if the active palette is not defined:
kibana/src/plugins/vis_default_editor/public/components/controls/palette_picker.tsx
Line 64 in 18514ce
valueOfSelected={activePalette?.name || DEFAULT_PALETTE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Thank you :)
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kibana Vis Editors team changes LGTM. I tested it locally and works fine, also it is bwc ;)
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/results/get_anomalies_table_data·ts.apis Machine Learning ResultsService GetAnomaliesTableData should fetch anomalies table dataStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @Kunzetsov |
* Added arguments to Tagcloud at Canvas. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Completes a part of #101377.
At this PR
TagCloud
side panel is added toCanvas
.What is added/changed:
view
for thetagcloud
expression was added.vis_dimension
argument
was added.palette
argument oftagcloud
function was changed fromstring
topalette
.tagcloud
to_ast
was changed.default
palette totagcloud
fn was changed.