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

[XY] xyVis and layeredXyVis. #128255

Merged

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented Mar 22, 2022

Completes #128056 and part of #127115


Changes made:

  • layeredXyVis and xyVis expressions are splitted.
  • title and description arguments are removed from layeredXyVis and xyVis.
  • DataLayers component is added and code has been simplified.
  • xyVis has changed to be used at Canvas. strict, default and required has been added to the arguments to make the expression use-friendly.
  • yConfig and extendedYConfig are splitted. yConfig is used for configuration of dataLayer and extendedDataLayer, it contains only options, which are appliable to the dataLayer. extendedYConfig contains all the configuration, which is appliable to the referenceLineLayer.
  • icon argument from referenceLineLayer should contain limited amount of available variants for user to be easy to use this expression.
  • Added y right and left extents validation.
  • Added legend configuration validation.
  • Added fillOpacity usage validation.
  • Added valueLabels usage validation. Removed not used options.
  • Added legendSize disabling at Lens, if legendPosition is inside.
  • Added validation for extent.mode, when it is turned to dataBounds and there are no line chart series.

Will be done in the separate PRs

  • referenceLineLayer should contain the additional argument for the static value. Otherwise, it is impossible for Canvas' users to use the referenceLineLayer expression via accessors, if they want some static value.
  • annotationLayers should become usable from Canvas. Now the annotation is not appearing in the chart if used with the other datasource expression function from the one, used in Lens.

Testing notes

  • layeredXyVis should be tested via Lens. As we've got rid of layerIds at the layeredXyVis, it was required to add some logic of remapping back dataLayer indexes to the layerIds of Lens, as this part of Kibana is tightly coupled with layerIds logic. So, should be checked if all the places, where activeData is used, it was remapped with the Visualization.convertActiveData.
  • Also, there have been added a lot of checks for the arguments, so, should be tested all the cases with different panel configurations and different series types. Lens must work as before, without errors from the expressions side. As we've got rid of
  • xyVis should be tested from Canvas with different variations of arguments (starting from zero arguments. The main goal of this PR was to make this expression usable for Canvas' users. It has to guide users on how to use it.). referenceLineLayers and annotationLayers are not usable from Canvas yet.

Example of the expression for the testing in Canvas

kibana
| selectFilter
| demodata
| head 10
| xyVis dataLayers={dataLayer xAccessor="project" accessors="price" seriesType="line"}
| render

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested around a bit more and couldn't find anything that doesn't work. LGTM!

if (!aliases?.length) {
throw new Error(`${fnDef.name} requires an argument`);
}

// use an alias if _ is the missing arg
const errorArg = name === '_' ? aliases[0] : name;
Copy link
Member

Choose a reason for hiding this comment

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

this line has a potential to throw (when aliases is not an array).

const errorArg = name === '_' && aliases?.length ? aliases[0] : name;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked it. name can't have the name _. It should be in aliases. So, in any case, you'll have the first element. Am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

in that case we should get rid of errorArg and just use name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked everything. Works as expected. That check was not used.

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

mbondyra commented May 2, 2022

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

mbondyra and others added 6 commits May 3, 2022 09:44
…pressions-xy-extended_layers

# Conflicts:
#	src/plugins/chart_expressions/expression_xy/common/expression_functions/annotation_layer.ts
#	src/plugins/chart_expressions/expression_xy/public/components/annotations.tsx
#	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/event_annotation/common/index.ts
#	src/plugins/event_annotation/common/manual_event_annotation/index.ts
#	src/plugins/event_annotation/common/types.ts
#	x-pack/plugins/lens/public/xy_visualization/xy_config_panel/annotations_config_panel/index.tsx
#	x-pack/plugins/lens/public/xy_visualization/xy_config_panel/color_picker.tsx
#	x-pack/plugins/lens/public/xy_visualization/xy_config_panel/reference_line_config_panel/reference_line_panel.tsx
#	x-pack/plugins/lens/public/xy_visualization/xy_config_panel/shared/icon_select.tsx
#	x-pack/plugins/lens/public/xy_visualization/xy_config_panel/shared/marker_decoration_settings.tsx
# Conflicts:
#	src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
eventAnnotation 11 12 +1
expressionXY 72 90 +18
lens 762 765 +3
total +22

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
eventAnnotation 87 90 +3
expressionXY 463 127 -336
lens 452 458 +6
total -327

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
expressionXY 43.4KB 55.5KB +12.1KB
lens 1.1MB 1.1MB +696.0B
total +12.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
expressionXY 0 13 +13
kibana 336 335 -1
lens 30 32 +2
total +14

Page load bundle

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

id before after diff
eventAnnotation 7.2KB 7.5KB +280.0B
expressions 98.9KB 98.8KB -112.0B
expressionXY 24.0KB 27.7KB +3.8KB
total +3.9KB
Unknown metric groups

API count

id before after diff
eventAnnotation 87 90 +3
expressionXY 473 137 -336
lens 527 534 +7
total -326

async chunk count

id before after diff
expressionXY 1 5 +4

History

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

cc @Kunzetsov

@Kuznietsov Kuznietsov merged commit b29b468 into elastic:main May 3, 2022
academo pushed a commit to academo/kibana that referenced this pull request May 4, 2022
* Added extended layers expressions.

* Added support of tables at layers.

* Added annotations to layeredXyVIs.

* Refactored the implementation to be reusable.

* Fixed undefined layers.

* Fixed empty arrays problems.

* Fixed input translations and removed not used arguments.

* Fixed missing required args error, and added required to arguments.

* Simplified expression configuration.

* Added strict to all the expressions.

* Moved dataLayer to the separate component.

* Refactored dataLayers helpers and xy_chart.

* fillOpacity usage validation is added.

* Fixed valueLabels argument options. Removed not used. Added validation for usage.

* Added validation to the layeredXyVis.

* Fixed extent validation.

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
* Added extended layers expressions.

* Added support of tables at layers.

* Added annotations to layeredXyVIs.

* Refactored the implementation to be reusable.

* Fixed undefined layers.

* Fixed empty arrays problems.

* Fixed input translations and removed not used arguments.

* Fixed missing required args error, and added required to arguments.

* Simplified expression configuration.

* Added strict to all the expressions.

* Moved dataLayer to the separate component.

* Refactored dataLayers helpers and xy_chart.

* fillOpacity usage validation is added.

* Fixed valueLabels argument options. Removed not used. Added validation for usage.

* Added validation to the layeredXyVis.

* Fixed extent validation.

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
@jdmcalee
Copy link

jdmcalee commented Nov 1, 2023

I apologize for adding this comment now, but I'm trying to understand something. With this change, the ability to scale axes to data bounds was removed for bar charts. Could anyone explain why that was done? My company has several visualizations that became unusable because of that behavior change.

@dej611
Copy link
Contributor

dej611 commented Nov 1, 2023

@jdmcalee we're aware of that and looking into fixing it: #170316

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Lens impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:x-large Extra Large Level of Effort release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.