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

[Lens] Fix transition to custom palette inconsistency when in number mode #110852

Merged
merged 5 commits into from
Sep 3, 2021

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Sep 1, 2021

Summary

Fixes #110771

When the palette configuration is in number mode and the user transitions from a default palette to a custom one, all cells were coloured with the last color stop's value.

While fixing this issue, I found out there was also another issue in palette transition when the continuity was not set to above.

This PR addresses both issues.

table_fixed

table_fixed_continuity

I've added some extra tests as all this code is pretty complicated and we had issues in the past of inconsistencies.

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added v8.0.0 Feature:Lens auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 1, 2021
@dej611 dej611 marked this pull request as ready for review September 2, 2021 09:28
@dej611 dej611 requested a review from a team as a code owner September 2, 2021 09:28
@elasticmachine
Copy link
Contributor

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

@dej611
Copy link
Contributor Author

dej611 commented Sep 2, 2021

@elasticmachine merge upstream

@dej611
Copy link
Contributor Author

dej611 commented Sep 2, 2021

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

// as of max reduce its value by 1/colors.length for correct continuity checks
const maxRange = stops.length
? rangeMax
: dataRangeArguments[1] - (dataRangeArguments[1] - dataRangeArguments[0]) / colors.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be just dataRangeArguments[0] / colors.length? I mean:
x-(x-y) = x-x+y = y.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The division has precedence over subtraction.

It basically does x - (x-y)/z (i.e. 100 - (100 - 0)/5 => 100 - 20 = 80)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought it is in the brackets, nvm then!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
charts 56.2KB 56.2KB +60.0B
lens 1.6MB 1.6MB +173.0B
total +233.0B

History

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

@mbondyra
Copy link
Contributor

mbondyra commented Sep 3, 2021

Tested on FF and couldn't reproduce the bug from the issue. I left one comment but overall this looks great!

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.

Code LGTM, bug is fixed 👍

@dej611
Copy link
Contributor Author

dej611 commented Sep 3, 2021

Do you think it makes sense to port it to 7.15.1 as well?
cc @stratoula @mbondyra

@stratoula
Copy link
Contributor

Yes, why not?

@dej611 dej611 added the v7.15.1 label Sep 3, 2021
@dej611 dej611 merged commit 21b4752 into elastic:master Sep 3, 2021
@dej611 dej611 deleted the fix/110771 branch September 3, 2021 13:57
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 3, 2021
…mode (elastic#110852)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 3, 2021
…mode (elastic#110852)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x
7.15

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 3, 2021
…mode (#110852) (#111144)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
kibanamachine added a commit that referenced this pull request Sep 3, 2021
…mode (#110852) (#111145)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.15.1 v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Passing from default to custom palette, when using numeric stops, breaks the coloring
5 participants