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] Fix layer-flash when changing style #80948

Merged
merged 2 commits into from
Oct 19, 2020
Merged

[Maps] Fix layer-flash when changing style #80948

merged 2 commits into from
Oct 19, 2020

Conversation

thomasneirynck
Copy link
Contributor

Summary

Closes #80946.

This was introduces by #78490. This was an oversight on my part. While the style-spec separates compound property-names with a -, the programmable object in JS in mapbox-gl is camel-cased.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@thomasneirynck thomasneirynck requested a review from a team as a code owner October 19, 2020 04:56
@thomasneirynck thomasneirynck added release_note:fix release_note:skip Skip the PR/issue when compiling release notes [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation labels Oct 19, 2020
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
maps 3.2MB 3.2MB -4.0B

History

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

@nreese
Copy link
Contributor

nreese commented Oct 19, 2020

What about all of the other uses of source-layer in vector_layer.tsx?

Screen Shot 2020-10-19 at 7 27 02 AM

nreese
nreese reviewed Oct 19, 2020
View reviewed changes
x-pack/plugins/maps/public/classes/layers/tiled_vector_layer/tiled_vector_layer.tsx
if (mbLayer && mbLayer['source-layer'] !== tiledSourceMeta.layerName) {
// The mapbox type in the spec is specified with `source-layer`
// but the programmable JS-object uses camelcase `sourceLayer`
// @ts-expect-error
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind creating a PR against https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/mapbox-gl to fix https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mapbox-gl/index.d.ts#L1891?

@thomasneirynck
Copy link
Contributor Author

@nreese looking at this deeper, the uses in vector_layer.tsx of source-layer are against a static layer-object, part the mapboxstyle-spec, and need to be source-layer. This is just a json data-structure.

The bug here is that sourceLayer is camel-cased in the actual JS-object, which has methods etc... Not 100% sure if this mismatch is considered a bug in mapbox or not.

It's possible sourceLayer is an internal undocumented property that we should not be using.

Will investigate.

@thomasneirynck
Copy link
Contributor Author

This mismatch is actually by design:

see mapbox/mapbox-gl-js#4163

This was flagged as a breaking-change: mapbox/mapbox-gl-js#6584

I am not sure where this leaves us with filing an upstream bug-fix for https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mapbox-gl/index.d.ts#L1891. Imho, that one correctly types the JSON mapbox-style spec, not the mapbox-gl-js API.

My suggestion would be to leave as-is, as usage is not incorrect. Rather, it is an undocumented part of the mapbox-gl-js api. Not sure if it's worth rewriting this Maps-code to not use the JS-object, and rather retrieve it via the style-sheet.

nreese
nreese approved these changes Oct 19, 2020
View reviewed changes
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.

LGTM. Thanks for checking into the other uses and about the typing errors.

@thomasneirynck thomasneirynck merged commit db14725 into elastic:master Oct 19, 2020
@thomasneirynck thomasneirynck mentioned this pull request Oct 19, 2020
Merged
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Oct 19, 2020
@thomasneirynck
[Maps] Fix layer-flash when changing style (elastic#80948)
ff6ef38
@thomasneirynck thomasneirynck mentioned this pull request Oct 19, 2020
Merged
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Oct 19, 2020
@thomasneirynck
[Maps] Fix layer-flash when changing style (elastic#80948)
2d51f68
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 20, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/cloud-cal…
b281379 
…lout-for-warm-and-cold-tier

* 'master' of github.com:elastic/kibana: (126 commits)
  Add cumulative sum expression function (elastic#80129)
  [APM] Fix link to trace (elastic#80993)
  Provide url rewritten in onPreRouting interceptor (elastic#80810)
  limit renovate to npm packages
  Fix bug in logs UI link (elastic#80943)
  [Monitoring] Fix bug with setup mode appearing on pages it shouldn't (elastic#80343)
  [Security Solution][Detection Engine] Fixes false positives caused by empty records in threat list
  docs test (elastic#81080)
  Fixed alerts ui test timeout issue, related to the multiple server calls for delete all alerts, by reducing the number of alerts to the two and increasing retry timeout. (elastic#81067)
  [APM] Fix service map highlighted edge on node select (elastic#80791)
  Fix typo in toast, slight copy adjustment. (elastic#80843)
  [Security Solution] reduce optimizer limits (elastic#80997)
  [maps] 7.10 documentation updates (elastic#79917)
  [Workplace Search] Fix Group Prioritization route and clean up design (elastic#80903)
  [Enterprise Search] Added reusable HiddenText component to Credentials (elastic#80033)
  Upgrade EUI to v29.5.0 (elastic#80753)
  [Maps] Fix layer-flash when changing style (elastic#80948)
  [Security Solution] [Detections] Disable edit button when user does not have actions privileges w/ rule + actions (elastic#80220)
  [Enterprise Search] Handle loading state on Credentials page (elastic#80035)
  [Monitoring] Fix cluster listing page in how it handles global state (elastic#78979)
  ...
thomasneirynck added a commit that referenced this pull request Oct 20, 2020
@thomasneirynck
[Maps] Fix layer-flash when changing style (#80948) (#81059)
f844a30
thomasneirynck added a commit that referenced this pull request Oct 20, 2020
@thomasneirynck
[Maps] Fix layer-flash when changing style (#80948) (#81061)
2de419b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreese nreese nreese approved these changes

@kindsun kindsun Awaiting requested review from kindsun

Assignees
No one assigned
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:fix release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Maps] Layer flashes when changing styling params
4 participants
@thomasneirynck @elasticmachine @kibanamachine @nreese