PB-1493 : Add iframe parameter to scroll / zoom with CTRL#1283
Merged
sami-nouidri-swisstopo merged 1 commit intodevelopfrom Apr 10, 2025
Merged
Conversation
web-mapviewer
|
||||||||||||||||||||||||||||
| Project |
web-mapviewer
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 01m 39s |
| Commit |
|
| Committer | Sami Nouidri |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
1
|
Skipped
|
0
|
Passing
|
48
|
| View all changes introduced in this branch ↗︎ | |
167d14a to
fbd94ff
Compare
51a0a1d to
b7d1823
Compare
ltkum
requested changes
Apr 7, 2025
Contributor
ltkum
left a comment
There was a problem hiding this comment.
In general, seems good, three things :
- The naming convention is not that great, it would be better if, within the code, we knew it was a parameter meant for the embed view, and that we knew it was for the zoom. Something in the form :
noSimpleZoomfor the url parameter andnoSimpleEmbedZoomfor the store. - I would add a button in the embed preview window to add or remove that parameter from the embed link, so people have a way to add the parameter.
- When I scroll on a page with the embed link, it stops the scrolling and focus on the map, stopping from going further down the page. It could be a good idea to find a way to stop the embedded viewer from overriding the main page scroll
packages/mapviewer/src/router/storeSync/ScrollingModeParamConfig.class.js
Outdated
Show resolved
Hide resolved
packages/mapviewer/src/router/storeSync/ScrollingModeParamConfig.class.js
Outdated
Show resolved
Hide resolved
3912b49 to
2a44ee3
Compare
ltkum
reviewed
Apr 7, 2025
Contributor
ltkum
left a comment
There was a problem hiding this comment.
We discussed a bit on slack, and translations should be coming soon.
One last thing: for the sake of consistency, It would be better to use a square button, since other options (like for example : in the print) are using this kind of button to be toggled.
2a44ee3 to
d0c0f40
Compare
ltkum
reviewed
Apr 8, 2025
Contributor
ltkum
left a comment
There was a problem hiding this comment.
Functionality seems good, now it's time for details and cleanup :)
I think our LLM has provided the translations in the ticket, if you want to add them :)
packages/mapviewer/src/router/storeSync/NoSimpleZoomParamConfig.class.js
Outdated
Show resolved
Hide resolved
packages/mapviewer/src/router/storeSync/NoSimpleZoomParamConfig.class.js
Outdated
Show resolved
Hide resolved
packages/mapviewer/src/modules/menu/components/share/MenuShareEmbed.vue
Outdated
Show resolved
Hide resolved
9742e17 to
abd2d48
Compare
ltkum
reviewed
Apr 10, 2025
Contributor
ltkum
left a comment
There was a problem hiding this comment.
We're getting there :)
only a few more details to go through :)
| const style = { 'max-width': iFrameWidth.value } | ||
| // Uses the width of the zoomModeLabel to set the width of the modal | ||
| // to avoid line breaks | ||
| const style = { 'min-width': `${Math.max(300, labelWidth.value)}px` } |
Contributor
There was a problem hiding this comment.
also add the max width of Math.max(labelWidth.value, iFrameWidth.value)
| class="form-check-label" | ||
| for="toggleZoomModeCheckbox" | ||
| > | ||
| {{ t('toggle_zooming_mode') }} |
Contributor
There was a problem hiding this comment.
The message on the box's tooltip is not the same as the one displayed on the map
| return | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
when the popup is displayed, ctrl+zooming when the mouse is on the popup does a browser zoom. If you can find a way to avoid that, that would be great. (an event listener on the ctrl key for example)
If it adds too much complexity, don't bother about it
| }) | ||
|
|
||
| onMounted(() => { | ||
| log.info(`Embedded map view mounted`) |
Contributor
There was a problem hiding this comment.
let's add that log again at the end :)
0b71722 to
fe93908
Compare
ltkum
approved these changes
Apr 10, 2025
Contributor
ltkum
left a comment
There was a problem hiding this comment.
LGTM. I'll add the translations in the google sheet :)
Issue : We wanted to add a way for people to have an iframe which did not zoom automatically when we were scrolling the page on which they were embedded. We also did not want to alter the default behavior, as we did not want thousands of mails Fix: We added a new URL parameter, and put it in the store, to alter the behavior of the scroll on embed views when it is active. We also added a button to toggle it when creating a new iframe. Added test for scrolling embedded preview & make modal window size dynamic
b1dd937 to
489d2bd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If the nosimplezoom = true parameter is added to an embedded URL, the user needs to press ctrl in order to zoom into the map. Here's a preview of the hint :
Test link