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] Reverse colors should not reverse palette picker previews #110455

Merged
merged 11 commits into from
Sep 6, 2021

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Aug 30, 2021

Summary

Fixes #110290

This PR is an attempt to address the #110290 problem, while changing a bit the palette panel behaviour:
when revering a color stops configuration, this PR makes an automatic transition of the palette from the current state to a custom palette.

This means that the current saved palette configuration with predefined palettes and reverse option should be migrated to the new configuration.
Because the palette picker is now showing predefined palettes always with a direction, selecting one of these palettes resets the reverse flag.

Screenshot 2021-08-30 at 12 56 01

  • On reverse transition to a custom palette
    • It would be nice to have the picker popover scroll to the current selection
  • On new palette selection from the palette picker resets the reverse flag
  • add a migration script to handle the new reverse state

Testing

  • Create a new datatable and with a predefined palette reverse the color stops

    • Check on the palette picker that all palettes have not been reversed and the current selection is now the Custom Palette
  • I've prepared a dashboard with 4 datatables using conditional coloring with a predefined palette (2 reversed + 2 regular) before the fix/migration (rename the extension from txt to ndjson and import it)

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Risk Probability Severity Mitigation/Notes
Migration should correctly work on current user datatables/heatmaps Medium Medium Unit tests will verify that migration will touch only specific datatables and heatmaps affected by the issue.

@dej611
Copy link
Contributor Author

dej611 commented Sep 3, 2021

@elasticmachine merge upstream

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

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

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.

maps changes LGTM
code review

@mbondyra
Copy link
Contributor

mbondyra commented Sep 6, 2021

@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

@dej611 thanks for preparing testing saved objects. I tested by migrating the provided samples and my own SO (screenshot) too, just to be sure. Tested on Chrome and migrations work as expected.
Screenshot 2021-09-06 at 11 59 27
Code LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

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
lens 228 229 +1

Async chunks

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

id before after diff
lens 1.6MB 1.6MB -191.0B
Unknown metric groups

API count

id before after diff
lens 246 247 +1

History

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

@dej611 dej611 merged commit 1a88d34 into elastic:master Sep 6, 2021
@dej611 dej611 deleted the fix/110290 branch September 6, 2021 12:27
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 6, 2021
…stic#110455)

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

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 6, 2021
…0455) (#111276)

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.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Reverse colors button incorrectly affecting color palette selector
5 participants