[XY] Defaults the point size to 1 and corrects the vislib migrations by stratoula · Pull Request #118994 · elastic/kibana

stratoula · 2021-11-18T08:24:00Z

Summary

This PR introduces the following changes:

Changes the default point size to be 1 instead of 3. This means that all the new visualizations will default to a smaller point size

Before

Now

Updates the xy migration script in order to add ciclesRadius attribute to 1 for all these visualizations that don't have the attribute (these will be visualizations made by vislib). All the visualizations that have been created with the new implementation have already this attribute and we don't change them.
Updates the migration script to add as a fitting function the linear for all these visualizations that haven't this attribute.

How to test

Create a new area/line visualization and check that the point size is 1
Create a visualization with vislib (for example from 7.11) and import it. The point size should be 1
Create a visualization with EC (for example from 7.15) and import it. The point size should be 3.

Here is a screenshot with a 7.11 area imported to this branch. The point size is set to 1

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…ations

elasticmachine · 2021-11-18T12:16:16Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

stratoula · 2021-11-18T12:42:03Z

@markov00 how do you feel about this change?
To wrap up

I am changing the default dot size from 3 to 1 to be in sync with lens (for the new visualizations)
I am migrating all the vislib old visualizations to size 1 (unless the users have specifically set this setting)
I am migrating all the vislib old visualizations to the linear fitting function (unless it already exists in the SO)

flash1293 · 2021-11-22T09:05:03Z

@elasticmachine merge upstream

markov00 · 2021-11-23T17:08:53Z

@markov00 how do you feel about this change? To wrap up

I am changing the default dot size from 3 to 1 to be in sync with lens (for the new visualizations)

I am migrating all the vislib old visualizations to size 1 (unless the users have specifically set this setting)

I am migrating all the vislib old visualizations to the linear fitting function (unless it already exists in the SO)

@stratoula @flash1293 Lens seems to hide the dots if I'm not wrong, or it was like that until 7.16.0. Was that changed in 8.0?

Why we are adding the linear fitting function? was that the default behaviour pre-elastic-charts?

stratoula · 2021-11-24T07:33:28Z

@elasticmachine merge upstream

stratoula · 2021-11-24T07:49:23Z

@markov00 about the linear. We did this change due to this sdh https://github.com/elastic/sdh-kibana/issues/2284. There is no fitting function that does exactly what vislib did. Until now, we were migrating them to the zero but this is not the best option for the customers. We thought that maybe the linear is the best approach here. I don't have a strong opinion tbh. If we could have the same behavior as the vislib, this would be the best.

About the dots. This is true, Lens doesn't have any dots. I just decided to display the dots but with size 1 because it is closer to Lens and because so far we were displaying the dots with size 3 so maybe if we hide them, it will be a bigger change for the users. Again, I don't have a strong opinion. I could also hide them.

What do you think is the best approach for these two issues?

stratoula · 2021-11-29T07:20:14Z

@elasticmachine merge upstream

stratoula · 2021-12-01T07:35:51Z

@elasticmachine merge upstream

stratoula · 2021-12-03T08:38:37Z

@elasticmachine merge upstream

dej611

Code review only 👍

markov00 · 2021-12-03T15:47:58Z

@markov00 about the linear. We did this change due to this sdh elastic/sdh-kibana#2284. There is no fitting function that does exactly what vislib did. Until now, we were migrating them to the zero but this is not the best option for the customers. We thought that maybe the linear is the best approach here. I don't have a strong opinion tbh. If we could have the same behavior as the vislib, this would be the best.

Linear is fine, we merged this few days ago elastic/elastic-charts#1505 to allow styling linear interpolation as it was a the original line

About the dots. This is true, Lens doesn't have any dots. I just decided to display the dots but with size 1 because it is closer to Lens and because so far we were displaying the dots with size 3 so maybe if we hide them, it will be a bigger change for the users. Again, I don't have a strong opinion. I could also hide them.

Ok, if we just migrate old vislib saved viz to new ech version with dotsize of 1 I'm fine.

stratoula · 2021-12-06T07:00:54Z

@elasticmachine merge upstream

markov00

Code review only: looks good to me, probably you need to fix one the condition I've commented on.

src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.ts

src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.test.ts

…ed_object_migrations.test.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

…ed_object_migrations.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

kibana-ci · 2021-12-06T17:13:46Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #11308 failed 11d07ca
💚 Build #11183 succeeded 18205ff
💚 Build #10877 succeeded 7c1f651
💚 Build #10249 succeeded 8d0293d
💚 Build #9495 succeeded b102aaf

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

…lastic#118994) * [XY] Changes the default point size to 1 and corrects the vislib migrations * Fixes the unit test * Change the migration function to default te fitting function to linear * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.test.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Fixes on PR review Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

kibanamachine · 2021-12-08T07:03:00Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.16

The backport PRs will be merged automatically after passing CI.

…118994) (#120717) * [XY] Changes the default point size to 1 and corrects the vislib migrations * Fixes the unit test * Change the migration function to default te fitting function to linear * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.test.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Fixes on PR review Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

…118994) (#120718) * [XY] Changes the default point size to 1 and corrects the vislib migrations * Fixes the unit test * Change the migration function to default te fitting function to linear * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.test.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Fixes on PR review Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

…lastic#118994) * [XY] Changes the default point size to 1 and corrects the vislib migrations * Fixes the unit test * Change the migration function to default te fitting function to linear * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.test.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Update src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com> * Fixes on PR review Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

[XY] Changes the default point size to 1 and corrects the vislib migr…

53b8af3

…ations

stratoula changed the title ~~[XY] Changes the default point size to 1 and corrects the vislib migr…~~ [XY] Defaults the point size to 1 and corrects the vislib migrations Nov 18, 2021

stratoula added Feature:XYAxis XY-Axis charts (bar, area, line) v7.16.1 v8.0.0 v8.1.0 release_note:fix labels Nov 18, 2021

stratoula added 2 commits November 18, 2021 11:40

Fixes the unit test

abefb3f

Change the migration function to default te fitting function to linear

66e5206

stratoula added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Nov 18, 2021

stratoula marked this pull request as ready for review November 18, 2021 12:16

stratoula requested a review from a team as a code owner November 18, 2021 12:16

Merge branch 'main' into migrate-circle-radius

0b976f2

Merge branch 'main' into migrate-circle-radius

68a76cf

Merge branch 'main' into migrate-circle-radius

b102aaf

Merge branch 'main' into migrate-circle-radius

8d0293d

Merge branch 'main' into migrate-circle-radius

7c1f651

dej611 approved these changes Dec 3, 2021

View reviewed changes

Merge branch 'main' into migrate-circle-radius

18205ff

markov00 reviewed Dec 6, 2021

View reviewed changes

src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.ts Show resolved Hide resolved

src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.test.ts Outdated Show resolved Hide resolved

stratoula and others added 3 commits December 6, 2021 15:22

Update src/plugins/visualizations/server/migrations/visualization_sav…

37072a9

…ed_object_migrations.test.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

Update src/plugins/visualizations/server/migrations/visualization_sav…

11d07ca

…ed_object_migrations.ts Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

Fixes on PR review

3c21376

stratoula added the auto-backport Deprecated - use backport:version if exact versions are needed label Dec 6, 2021

stratoula merged commit 82979a3 into elastic:main Dec 8, 2021

kibanamachine mentioned this pull request Dec 8, 2021

[8.0] [XY] Defaults the point size to 1 and corrects the vislib migrations (#118994) #120717

Merged

kibanamachine mentioned this pull request Dec 8, 2021

[7.16] [XY] Defaults the point size to 1 and corrects the vislib migrations (#118994) #120718

Merged

Conversation

stratoula commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How to test

Checklist

Uh oh!

elasticmachine commented Nov 18, 2021

Uh oh!

stratoula commented Nov 18, 2021

Uh oh!

flash1293 commented Nov 22, 2021

Uh oh!

markov00 commented Nov 23, 2021

Uh oh!

stratoula commented Nov 24, 2021

Uh oh!

stratoula commented Nov 24, 2021

Uh oh!

stratoula commented Nov 29, 2021

Uh oh!

stratoula commented Dec 1, 2021

Uh oh!

stratoula commented Dec 3, 2021

Uh oh!

dej611 left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 commented Dec 3, 2021

Uh oh!

stratoula commented Dec 6, 2021

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibana-ci commented Dec 6, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

kibanamachine commented Dec 8, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stratoula commented Nov 18, 2021 •

edited

Loading