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

PB-1254: fix embed preview in chrome #1159

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

ltkum
Copy link
Contributor

@ltkum ltkum commented Dec 3, 2024

Issue: In chrome-based web browsers, the layers would be removed from the preview and the link when opening the embed preview unless the bgLayer was 'void'.

Cause: There might be a need to refactor completely the embed sharing component, but for now, it seems the issue was caused by the bgLayer parameter being handled before the layers. This would cause a situation where the router would trigger a mutation, go to a new URL with no layers, and overwrite what should have been in store. As the default background layer is the void one, we did not go through this when we used the empty background layer.

Fix: by changing the order in which the parameters are handled, we can ensure layers are handled before the background layer, and thus ensure they are kept.

Test link

@ltkum ltkum requested a review from pakb December 3, 2024 14:21
@github-actions github-actions bot added the bug label Dec 3, 2024
Issue: In chrome-based web browsers, the layers would be removed from the preview and the link when opening the embed preview unless the bgLayer was 'void'.

Cause: There might be a need to refactor completely the embed sharing component, but for now, it seems the issue was caused by the bgLayer parameter being handled before the layers.
This would cause a situation where the router would trigger a mutation, go to a new URL with no layers, and overwrite what should have been in store. As the default background layer is the void one, we did not go through this when we used the empty background layer.

Fix: by changing the order in which the parameters are handled, we can ensure layers are handled before the background layer, and thus ensure they are kept.
@ltkum ltkum force-pushed the fix-PB-1254-allow-embed-layer-sharing branch from da3c4d9 to f10e2ca Compare December 3, 2024 14:27
Copy link

cypress bot commented Dec 3, 2024

web-mapviewer    Run #4021

Run Properties:  status check passed Passed #4021  •  git commit 772af64d10: PB-1254: fix embed test
Project web-mapviewer
Branch Review fix-PB-1254-allow-embed-layer-sharing
Run status status check passed Passed #4021
Run duration 05m 17s
Commit git commit 772af64d10: PB-1254: fix embed test
Committer Martin Künzi
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 214
View all changes introduced in this branch ↗︎

Copy link
Contributor

Choose a reason for hiding this comment

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

can you change all your invoke(...).then(...) into invoke(...).should(...) ? This would add retry-ability to all your tests (and should make them a bit more resilient on the CI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Issue : the tests for the embed links checked strings as they are without any further processing. This meant that if the order of the parameters changed, it would make the test fail.

Fix : We update the tests with a deep comparison between two 'URLSearchParams' made from the variable parameter and the validation string
@ltkum ltkum force-pushed the fix-PB-1254-allow-embed-layer-sharing branch from bcf2432 to 772af64 Compare December 6, 2024 09:38
@ltkum ltkum requested a review from pakb December 6, 2024 10:11
@ltkum ltkum merged commit 6b55ef1 into develop Dec 6, 2024
6 checks passed
@ltkum ltkum deleted the fix-PB-1254-allow-embed-layer-sharing branch December 6, 2024 11:47
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.

2 participants