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

Vislib visualization renderer #80744

Merged
merged 28 commits into from
Oct 29, 2020
Merged

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Oct 15, 2020

Summary

Part of #46801

Implement a custom renderer for table visualization. This PR includes next changes:

  • implement toExpressionAst function for all vislib types and separate function for pie type.
  • implement a single custom legacy renderer for all vislib types
  • fix unit tests;
  • enhanced types;
  • lazy load components for the editor - reduce the initial bundle size;

Before

image

After

image

Checklist

@nickofthyme nickofthyme requested a review from sulemanof October 15, 2020 20:04
@nickofthyme nickofthyme added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Vislib Vislib chart implementation release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0 labels Oct 15, 2020
@nickofthyme nickofthyme mentioned this pull request Oct 15, 2020
13 tasks
@nickofthyme nickofthyme marked this pull request as ready for review October 21, 2020 17:02
@nickofthyme nickofthyme requested a review from a team October 21, 2020 17:02
@nickofthyme nickofthyme requested a review from a team as a code owner October 21, 2020 17:02
@elasticmachine
Copy link
Contributor

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

@nickofthyme
Copy link
Contributor Author

nickofthyme commented Oct 27, 2020

@elastic/kibana-design and @elastic/kibana-app could I get a review please 🙏

Copy link
Contributor

@sulemanof sulemanof 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 locally all affected vis types in Chrome & Firefox. Didn't find any regressions.

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

@nickofthyme, I tested locally and everything seems to work as expected. There's only one minor scss change that you can do.

@import './vislib/index'
@import './vislib/index';

.vislib .visLegend__toggle {
Copy link
Contributor

@elizabetdev elizabetdev Oct 28, 2020

Choose a reason for hiding this comment

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

These styles can be moved to src/plugins/vis_type_vislib/public/vislib/components/legend/_legend.scss.

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
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.

It looks great 👏 I have some minor comments that mainly have to do to remove lodash but it is LGTM. Tested locally on Safari and can't find any regression. Great size reduction 🥳

interface RenderValue {
visConfig: VisParams;
visData: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use unknown instead here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const usedValueAxis = (visConfig.valueAxes || []).find(
(valueAxis) => valueAxis.id === seriesParam.valueAxis
);
if (get(usedValueAxis, 'scale.mode') === 'percentage') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid import get from lodash here and use typescript instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yDimension.format = { id: 'percent' };
}
}
if (get(visConfig, 'gauge.percentageMode') === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too, in that way we don't need to use lodash at all 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mountLegend(visData: any, position: Positions) {
this.unmount = mountReactNode(
mountLegend(
visData: any,
Copy link
Contributor

Choose a reason for hiding this comment

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

And I think this could be unknown too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return false;
}

const rows: object[] | undefined = get(visData, 'rows');
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to not use lodash here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickofthyme
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
visTypeVislib 275 280 +5

async chunk count

id before after diff
visTypeVislib 1 7 +6

async chunks size

id before after diff
visTypeTagcloud 284.5KB 284.5KB +21.0B
visTypeVislib 538.0KB 700.3KB +162.2KB
total +162.3KB

distributable file count

id before after diff
default 48117 48134 +17
oss 28602 28619 +17

page load bundle size

id before after diff
visTypeTimelion 34.3KB 34.3KB -33.0B
visTypeVislib 207.9KB 66.1KB -141.9KB
visualizations 207.2KB 203.8KB -3.3KB
total -145.2KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Vislib Vislib chart implementation release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants