-
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
[XY] Mark size configuration. #130361
[XY] Mark size configuration. #130361
Conversation
…to chart_expressions-xy
# Conflicts: # src/plugins/chart_expressions/expression_xy/common/expression_functions/legend_config.ts # x-pack/plugins/lens/public/xy_visualization/visualization.tsx
…to chart_expressions-xy
# Conflicts: # src/plugins/chart_expressions/expression_xy/common/expression_functions/__snapshots__/xy_vis.test.ts.snap # src/plugins/chart_expressions/expression_xy/common/expression_functions/common_data_layer_args.ts # src/plugins/chart_expressions/expression_xy/common/expression_functions/common_xy_args.ts # src/plugins/chart_expressions/expression_xy/common/expression_functions/data_layer.test.ts # src/plugins/chart_expressions/expression_xy/common/expression_functions/data_layer.ts # src/plugins/chart_expressions/expression_xy/common/expression_functions/extended_data_layer_fn.ts # src/plugins/chart_expressions/expression_xy/common/expression_functions/layered_xy_vis_fn.ts # src/plugins/chart_expressions/expression_xy/common/expression_functions/validate.ts # src/plugins/chart_expressions/expression_xy/common/expression_functions/xy_vis.test.ts # src/plugins/chart_expressions/expression_xy/common/i18n/index.tsx # src/plugins/chart_expressions/expression_xy/common/types/expression_functions.ts # src/plugins/chart_expressions/expression_xy/public/components/xy_chart.test.tsx # src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx # src/plugins/chart_expressions/expression_xy/public/helpers/data_layers.tsx
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.
LGTM!
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.
LGTM!
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
@elastic/kibana-vis-editors, could you, please, review this PR? Thanks ) |
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.
pointseries
has a size
argument - I think we should map it to the markSizeAccessor by default, just like x and y.
Otherwise looks good to me (see comment about default marksize ratio)
src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
Outdated
Show resolved
Hide resolved
@flash1293, added the |
💚 Build SucceededMetrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @Kunzetsov |
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.
LGTM, tested and works as expected
Summary
Completes part of #127115
Added
markSizeAccessor
to thexyVis
andextendedDataLayer
expressions.Added
markSizeRatio
to thexyVis
andlayeredXyVis
expressions.markSizeAccessor
is specifiedmarkSizeAccessor
visdimension
with specific formatmarkSizeRatio
is specifiedmarkSizeAccessor
is specified for the series, different fromarea*
andline
charts.markSizeAccessor
is pointing to non-existent columnmarkSizeRatio
is greater then 100 and lower than 0pointseries