Skip to content

[Canvas] Fix bug with plot sorting#98084

Merged
crob611 merged 1 commit intoelastic:masterfrom
crob611:plot-sorting-bug
Apr 23, 2021
Merged

[Canvas] Fix bug with plot sorting#98084
crob611 merged 1 commit intoelastic:masterfrom
crob611:plot-sorting-bug

Conversation

@crob611
Copy link
Copy Markdown
Contributor

@crob611 crob611 commented Apr 22, 2021

Fixes #62770

Summary

In our plot function, we are taking in the input and doing a sortBy(input.rows, ['x', 'y', 'color', 'size', 'text'])

In Lodash 3.10, the sortBy method does not accept an array of properties to sort by as a second argument. That should have been done by sortByAll. 3.10 handles it, and does not sort the array.

Lodash 4 brought the functionality of sortByAll into the sortBy function, so where this line previously was a no-op, after the upgrade to 4 it started to always sort a plot by x,y,color,size,text

To fix this, I'm taking out the sortBy in the plot (and in the tick finding method as well) since when everything was working as it should, those were actually noops. This will allow the user to sort however they want and pass that into plot and have everything be sorted as expected.

@crob611 crob611 requested a review from a team as a code owner April 22, 2021 19:50
@crob611 crob611 added release_note:fix v7.13.0 v7.14.0 v8.0.0 Feature:Canvas loe:small Small Level of Effort review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// labels Apr 22, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@crob611 crob611 added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Apr 22, 2021
Copy link
Copy Markdown
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

YES!!!

@clintandrewhall clintandrewhall added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 22, 2021
@kibanamachine
Copy link
Copy Markdown
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
canvas 1.2MB 1.2MB -195.0B

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

@crob611 crob611 merged commit 4b42713 into elastic:master Apr 23, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 23, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 23, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 23, 2021
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Backport successful

Status Branch Result
7.12
7.13
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 23, 2021
Co-authored-by: Corey Robertson <corey.robertson@elastic.co>
kibanamachine added a commit that referenced this pull request Apr 23, 2021
Co-authored-by: Corey Robertson <corey.robertson@elastic.co>
kibanamachine added a commit that referenced this pull request Apr 23, 2021
Co-authored-by: Corey Robertson <corey.robertson@elastic.co>
@LeeDr LeeDr added v7.12.2 and removed v7.12.1 labels May 13, 2021
@LeeDr
Copy link
Copy Markdown

LeeDr commented May 13, 2021

I changed the v7.12.1 label to 7.12.2. I don't think the backport to 7.12 branch made it into the 7.12.1 release build.

This is the release commit for 7.12.1 https://github.com/elastic/kibana/commits/abb04c5543d2201630c9669fe3680864506d924e
on Apr 20, 2021
but it looks like this PR was merged to 7.12 branch on Apr 23, 2021.

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:Canvas impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:fix review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.12.2 v7.13.0 v7.14.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] plot expressions breaks data sorting

5 participants