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.

Sign up for GitHub

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

Jump to bottom

[Lens] lens_multitable removal. #131699

Merged
merged 18 commits into from
May 11, 2022
Merged

[Lens] lens_multitable removal. #131699

merged 18 commits into from
May 11, 2022

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented May 6, 2022 •
edited
Loading

Summary

  • Removed lens_multitable expression and shouldBuildDatasourceExpressionManually function from the Visualization interface, as was mentioned at the comment.
  • Added the switch to gauge at the functional test to make it isolated and repeatable.

@Kuznietsov Kuznietsov added WIP Work in progress Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels May 6, 2022
@Kuznietsov Kuznietsov requested a review from alexwizp May 6, 2022 09:41
@Kuznietsov Kuznietsov self-assigned this May 6, 2022
@Kuznietsov Kuznietsov added loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. v8.3.0 backport:skip This commit does not require backporting and removed WIP Work in progress labels May 10, 2022
@Kuznietsov Kuznietsov requested a review from flash1293 May 10, 2022 07:46
@Kuznietsov Kuznietsov requested a review from crob611 May 10, 2022 07:46
@Kuznietsov Kuznietsov marked this pull request as ready for review May 10, 2022 07:46
@Kuznietsov Kuznietsov requested review from a team as code owners May 10, 2022 07:46
@Kuznietsov Kuznietsov requested a review from VladLasitsa May 10, 2022 07:46
@flash1293 flash1293 mentioned this pull request May 10, 2022
Merged
flash1293
flash1293 reviewed May 10, 2022
View reviewed changes
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.

Looks mostly good to me, just one thing that looks a little suspicious

x-pack/test/functional/apps/lens/group3/gauge.ts
@@ -48,6 +48,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
});

it('should reflect edits for gauge', async () => {
await PageObjects.lens.switchToVisualization('horizontalBullet', 'gauge');
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these changes about? It should work just fine without this change, right?

Copy link
Contributor Author

@Kuznietsov Kuznietsov May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you'll run the test via grep by picking the current test case, it will fail. According to the FIRST principle of testing, a test should set up its own data and should not depend on any external factors to run its test. So, that is not really the right behavior, if the test is failing if ran separately from the previous test case. @flash1293, WDYT?

VladLasitsa
VladLasitsa approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

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

Code LGTM

flash1293
flash1293 approved these changes May 10, 2022
View reviewed changes
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.

LGTM, thanks for the explanation.

I'm not opposed to "drive by" improvements like this per se, but please call them out in the description, this makes it much easier to follow the changes.

@Kuznietsov
Copy link
Contributor Author

Kuznietsov commented May 10, 2022 •
edited
Loading

LGTM, thanks for the explanation.

I'm not opposed to "drive by" improvements like this per se, but please call them out in the description, this makes it much easier to follow the changes.

@flash1293, @VladLasitsa, thanks for the approval 🎉 .

Sure, will do that in the future.

@Kuznietsov
Merge branch 'main' into lens-multitable-removal
6e01a31 
# Conflicts:
#	x-pack/plugins/lens/public/datatable_visualization/components/table_actions.ts
#	x-pack/plugins/lens/public/xy_visualization/to_expression.ts
@Kuznietsov
Copy link
Contributor Author

@elastic/kibana-gis, could you, please, review this PR. Thanks 😃

nreese
nreese approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-gis changes LGTM
code review, tested in chrome

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 767 764 -3

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
expressionXY 126 122 -4
lens 457 449 -8
total -12

Async chunks

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

id before after diff
expressionXY 55.9KB 55.9KB +80.0B
lens 1.1MB 1.1MB +7.4KB
maps 2.5MB 2.5MB +34.0B
total +7.5KB

Page load bundle

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

id before after diff
expressionGauge 14.3KB 14.4KB +80.0B
expressionHeatmap 14.3KB 14.4KB +80.0B
expressionMetricVis 8.4KB 8.5KB +80.0B
expressionPartitionVis 22.7KB 23.0KB +320.0B
lens 32.6KB 27.5KB -5.1KB
maps 70.2KB 70.3KB +49.0B
total -4.5KB
Unknown metric groups

API count

id before after diff
expressionXY 136 132 -4
lens 533 524 -9
total -13

ESLint disabled line counts

id before after diff
lens 30 27 -3

Total ESLint disabled count

id before after diff
lens 32 29 -3

History

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

cc @Kunzetsov

@Kuznietsov Kuznietsov merged commit 040efae into elastic:main May 11, 2022
@flash1293 flash1293 mentioned this pull request May 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladLasitsa VladLasitsa VladLasitsa approved these changes

@flash1293 flash1293 flash1293 approved these changes

@nreese nreese nreese approved these changes

@alexwizp alexwizp Awaiting requested review from alexwizp

@crob611 crob611 Awaiting requested review from crob611

Assignees

@Kuznietsov Kuznietsov

Labels
backport:skip This commit does not require backporting Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Kuznietsov @kibana-ci @nreese @flash1293 @VladLasitsa