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

[Maps] fix layer sorting issue with hidden layers at map load #76007

Merged
merged 2 commits into from
Aug 27, 2020

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 26, 2020

fixes #75997

#69444 introduced a regression where hidden layers on map load could cause the sorting to place layers above the hidden layer beneath other layers. This occurred because beneathMbLayerId would be set to undefined when there were no mapbox layers for a layer. This PR resolves the issue by only advancing beneathMbLayerId if a mapbox layer could be found for the layer.

@nreese nreese requested a review from a team as a code owner August 26, 2020 16:53
@elasticmachine
Copy link
Contributor

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

@nreese
Copy link
Contributor Author

nreese commented Aug 27, 2020

@elasticmachine merge upstream

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 for fix!

@@ -128,7 +128,8 @@ export function syncLayerOrder(mbMap: MbMap, spatialFiltersLayer: ILayer, layerL
if (!isLayerInOrder(mbMap, mapLayer, LAYER_CLASS.LABEL, beneathMbLayerId)) {
moveMapLayer(mbMap, mbLayers, mapLayer, LAYER_CLASS.LABEL, beneathMbLayerId);
}
beneathMbLayerId = getBottomMbLayerId(mbLayers, mapLayer, LAYER_CLASS.LABEL);
const bottomMbLayerId = getBottomMbLayerId(mbLayers, mapLayer, LAYER_CLASS.LABEL);
if (bottomMbLayerId) beneathMbLayerId = bottomMbLayerId;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is really subtle. Do you mind adding a comment?

e.g.

When the corresponding bottom mb-layer cannot be found, - e.g. when the kibana-layer is invisible and has not synced with mapbox - retain the previous "beneath"-layer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented the unit test to capture the subtleness of the edge case. Do you think more commenting is needed?

@jsanz
Copy link
Member

jsanz commented Aug 27, 2020

Tested locally, works fine, thanks for the quick fix! 🎉

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
maps 3.3MB +110.0B 3.3MB

History

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

@nreese nreese merged commit b293961 into elastic:master Aug 27, 2020
nreese added a commit to nreese/kibana that referenced this pull request Aug 27, 2020
…or a layer (elastic#76007)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
nreese added a commit to nreese/kibana that referenced this pull request Aug 27, 2020
…or a layer (elastic#76007)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
nreese added a commit that referenced this pull request Aug 27, 2020
…or a layer (#76007) (#76087)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
nreese added a commit that referenced this pull request Aug 27, 2020
…or a layer (#76007) (#76086)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Visibility issue with WMS layers
5 participants