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

[maps] Use desaturated map tiles instead of bright map tiles by default #116179

Merged
merged 10 commits into from
Oct 27, 2021
Merged

[maps] Use desaturated map tiles instead of bright map tiles by default #116179

merged 10 commits into from
Oct 27, 2021

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Oct 25, 2021 •
edited
Loading

Fixes #115979

  • Adds lightModeDefault to EMSTMSSourceDescriptor.
  • EMSTMSSource.createDescriptor sets lightModeDefault to 'road_map_desaturated' for all newly created saved objects.
  • Migration sets lightModeDefault to 'road_map' for saved objects created before 8.0.0 so there are no breaking changes and existing maps will still use bright tiles.

@nreese nreese added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 reason:enhancement labels Oct 25, 2021
@nreese nreese requested a review from a team as a code owner October 25, 2021 17:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@nreese
Copy link
Contributor Author

nreese commented Oct 25, 2021

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Oct 26, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@nreese nreese mentioned this pull request Oct 26, 2021
Merged
@nreese
Copy link
Contributor Author

nreese commented Oct 26, 2021

@elasticmachine merge upstream

nickpeihl
nickpeihl approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm! tested in Chrome.

  • New maps are by default desaturated
  • Autoselect based on kibana theme still works
  • Imported saved objects from older version (7.15.1) maintain their original layer style

thomasneirynck
thomasneirynck reviewed Oct 26, 2021
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment •
edited
Loading

Choose a reason for hiding this comment

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

+1 on code, some minor comments.

I don't feel strongly about this, but I think the initial app load looks "off".

before:

image

after:

image

I also wonder if any user has been truly disadvantaged by the current "bright" base-map as a default on startup.

Anyways, +1 on merging if there's an overall preference for this new base map. cc @jsanz @nickpeihl @ghudgins @miukimiu

x-pack/plugins/maps/common/descriptor_types/source_descriptor_types.ts Outdated Show resolved Hide resolved
jsanz
jsanz approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@jsanz jsanz left a comment

Choose a reason for hiding this comment

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

Tested in Chrome. Creating new maps and loading the sample datasets all come with the desaturated style. As @nickpeihl tested, importing the kibana sample flights saved object from a 7.15.0 instance keeps the original bright style.

I like this style as a default as it helps users focus on their data, as the basemap is usually here to provide spatial context.

@ghudgins
Copy link

❤️ i'm in agreement here as this gets out of the way of the data!

thomasneirynck
thomasneirynck approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

thx @ghudgins for weighing in, lgtm

@nreese
Copy link
Contributor Author

nreese commented Oct 27, 2021

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Oct 27, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #12 / detection engine api security and spaces enabled Detection exceptions data types and operators Rule exception operators for data type keyword "is in list" operator will return 3 results if we have two lists with an AND keyword === "word one" AND keyword === "word two" since we have an array

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
maps 2.7MB 2.7MB +114.0B

History

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

@nreese nreese merged commit 3b3c4b2 into elastic:master Oct 27, 2021
nkhristinin pushed a commit to nkhristinin/kibana that referenced this pull request Oct 28, 2021
@nreese @kibanamachine @nkhristinin
[maps] Use desaturated map tiles instead of bright map tiles by defau…
73806c4 
…lt (elastic#116179)

* [maps] default to desaturated tiles in light mode

* fix jest and functional tests

* eslint

* update migrations version expect

* review feedback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 29, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 116179 or prevent reminders by adding the backport:skip label.

@nreese nreese added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Oct 29, 2021
@nreese nreese mentioned this pull request Nov 2, 2021
Closed
@nreese nreese mentioned this pull request Mar 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsanz jsanz jsanz approved these changes

@nickpeihl nickpeihl nickpeihl approved these changes

@thomasneirynck thomasneirynck thomasneirynck approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use desaturated map instead of bright by default
7 participants
@nreese @elasticmachine @kibanamachine @ghudgins @jsanz @nickpeihl @thomasneirynck