PB-90: Reduce shortlink location popup calls tabs version #603

LukasJoss · 2024-01-15T13:59:35Z

cypress · 2024-01-15T14:07:33Z

Passing run #379 ↗︎

0	165	19	0	0

Details:

PB-90: Clean up code for PR
Project: web-mapviewer	Commit: `2605c07491`
Status: Passed	Duration: 04:45 💡
Started: Jan 25, 2024 12:32 PM	Ended: Jan 25, 2024 12:36 PM

Review all test suite changes for PR #603 ↗︎

tests/cypress/tests-e2e/mouseposition.cy.js

ltshb

I just did a quick functional test of the UI (no code review yet) and looks very promising. A few issues/adjustment to do are listed below:

Once the popup is open in the share tab, if you move the map around or change the map zoom, the URL parameter are updated, but this update is not reflected to the share link. So changes in the url parameter should trigger an update of the share link
The name of the share tab should be changed to Share position (the old viewer is using this traduction so you should find a traduction key)
The size of the window should be reduced, with both tab we have some white space on the right hand side.
I would add the Share link label to the short link input element like in the Share menu (personal preference)
The title of the popup window could be updated to the title of the tab (just an idea that needs to be experienced if it looks good)

LukasJoss · 2024-01-18T13:05:22Z

4. Share

LukasJoss · 2024-01-18T13:55:35Z

I just did a quick functional test of the UI (no code review yet) and looks very promising. A few issues/adjustment to do are listed below:

1. Once the popup is open in the share tab, if you move the map around or change the map zoom, the URL parameter are updated, but this update is not reflected to the share link. So changes in the url parameter should trigger an update of the share link

2. The name of the share tab should be changed to `Share position` (the old viewer is using this traduction so you should find a traduction key)

3. The size of the window should be reduced, with both tab we have some white space on the right hand side.

4. I would add the `Share link` label to the short link input element like in the `Share` menu (personal preference)

5. The title of the popup window could be updated to the title of the tab (just an idea that needs to be experienced if it looks good)

1.) I added the sharelink update upon zoom but did not add the update upon map movement since the shortlink generated will always be centered on the chosen location anyway(what should be the behaviour upon changing maps?). Also maybe it would make sense to have the zoom level of the created url to have a fixed level to reduce the amount of created shortlinks even further.

3.) Instead of changing the windows size I made the tab buttons a constant size too keep consistency in the looks between the different language, is that good - though the copy sign might need to be removed?

4.) I didn't quite understand what you mean here.

5.)I temporarily added this change but I am not convinced of it

ltshb

The results looks very nice.

Some code clean up and improvements are required see comment, but thanks for the effort.

ltshb · 2024-01-22T07:42:47Z

src/modules/map/components/LocationPopup.vue

+    copyTooltip.value = tippy(copyButton.value, {
+        content: i18n.t('copy_success'),
+        arrow: true,
+        placement: 'right',
+        trigger: 'manual',
+        onShow(instance) {
+            setTimeout(() => {
+                instance.hide()
+            }, 1000)
+        },
+    })


Here we should not make the tippy instance reactive. We should only use javascript plain object as reactive and as well in store. Unfortunately this is not always the case in our code, but we should make sure not to add more exception to this rule.
Adding complex object to reactive has some performance drawback and can lead to very subtle bugs.

Wo here you dan simply use a normal variable without ref

let copyTooltipInstance = null onMounted(()=>{ copyTooltipInstance = tippy(...) })

ltshb · 2024-01-22T07:45:16Z

src/modules/map/components/LocationPopup.vue

-    shareLinkUrl.value = `${location.origin}/#/map?${stringifyQuery(query)}`
-    shortenShareLink(shareLinkUrl.value)
+
+function showTooltip() {


Maybe showCopiedTooltip is more explicit

ltshb · 2024-01-22T07:47:08Z

src/modules/map/components/LocationPopup.vue

 }
-async function updateQrCode(url) {
+
+async function copyValue() {


Maybe use copyShareLink

ltshb · 2024-01-22T07:48:15Z

src/modules/map/components/LocationPopup.vue

+const newClickInfo = ref(true)
+const showEmbedSharing = ref(false)
+const requestClipboard = ref(false)
+const copied = ref(false)


Maybe shareLinkCopied

ltshb · 2024-01-22T07:50:39Z

src/modules/map/components/LocationPopup.vue

+                        @click="
+                            (selectedTab = 'position'),
+                                (showEmbedSharing = false),
+                                (newClickInfo = false)
+                        "


Would be better to put this code to a function, e.g. onPositionTabClick

src/modules/map/components/LocationPopup.vue

ltshb · 2024-01-22T08:21:03Z

src/modules/map/components/LocationPopupPosition.vue

+.location-popup {
+    @extend .clear-no-ios-long-press;


This class is not needed here, already part of the parent component

ltshb · 2024-01-22T08:27:05Z

src/modules/map/components/LocationPopupShare.vue

+})
+watch(currentLang, () => {
+    if (showEmbedSharing.value) {
+        setTimeout(() => updateShareLink(), 1)


Why the setTimeout here ? if you have a race condition it could be also solved using vue nextTick method

ltshb · 2024-01-22T08:27:21Z

src/modules/map/components/LocationPopupShare.vue

+})
+watch(zoom, () => {
+    if (showEmbedSharing.value) {
+        setTimeout(() => updateShareLink(), 1)


Dito as above

ltshb · 2024-01-22T08:30:55Z

src/modules/map/components/LocationPopup.vue

+                <LocationPopupShare
+                    :class="{ active: selectedTab === 'share', show: selectedTab === 'share' }"
+                    :coordinate="coordinate"
+                    :click-info="clickInfo"
+                    :current-lang="currentLang"
+                    :show-embed-sharing="showEmbedSharing"
+                    @share-link="(i) => (shareLink = i)"
                />


I found the logic of the share link with the child event and the show-embed-sharing hard to follow, might be better to put the shortlink logic in the parent vue, meaning in this component LocationPopup and to pass the shortlink to the child component, might make the code simpler to follow.

ltshb

Very nice just a few minor comments but otherwise its all good.

ltshb · 2024-01-24T18:25:37Z

src/modules/map/components/LocationPopup.vue

@@ -1,45 +1,39 @@
 <script setup>
 /** Right click pop up which shows the coordinates of the position under the cursor. */

-import proj4 from 'proj4'
-import { computed, onMounted, ref, watch } from 'vue'
+// importing directly the vue component, see https://github.com/ivanvermeyen/vue-collapse-transition/issues/5


The comment is probably a copy past mistake

ltshb · 2024-01-24T18:26:49Z

src/modules/map/components/LocationPopup.vue

-const route = useRoute()
 const store = useStore()

 const clickInfo = computed(() => store.state.map.clickInfo)
-const currentLang = computed(() => store.state.i18n.lang)
 const projection = computed(() => store.state.position.projection)
 const showIn3d = computed(() => store.state.cesium.active)
+const currentLang = computed(() => store.state.i18n.lang)
+
+const route = useRoute()


Just code style I would keep the useRoute above with the other use...

ltshb · 2024-01-24T18:29:46Z

src/modules/map/components/LocationPopup.vue

+                updateShareLink()
+            }
+        }
+        console.log('Query parameters changed:', newQuery)


No console, if you want to keep some debugging log, you need to use log.debug

ltshb · 2024-01-24T18:35:38Z

src/modules/map/components/LocationPopup.vue

+const selectedTab = ref('position')
+const shareTabButton = ref(null)
+const newClickInfo = ref(true)
+const showEmbedSharing = ref(false)


This could be simplified by using a computed property

const showEmbedSharing = computed(()=> selectedTab.value === 'share')

So you don't have to set it and clear it below

ltshb · 2024-01-24T18:40:33Z

src/modules/map/components/LocationPopup.vue

 </script>

 <template>
    <component
        :is="mappingFrameworkSpecificPopup"
        v-if="coordinate"
-        :title="$t('position')"
+        :title="selectedTab == 'position' ? $t('position') : $t('link_bowl_crosshair')"


use the new style for translation i18n.t instead of $t

ltshb · 2024-01-24T18:41:02Z

src/modules/map/components/LocationPopupPosition.vue

+                :value="heightInMeter"
+                :extra-value="heightInFeet"
+            >
+                <a :href="$t('elevation_href')" target="_blank">{{ $t('elevation') }}</a>


ltshb · 2024-01-24T18:41:45Z

src/modules/map/components/LocationPopupShare.vue

+<style lang="scss" scoped>
+@import 'src/scss/webmapviewer-bootstrap-theme';
+</style>


Because the style is not used it can be removed

…ewhere

… present

github-actions bot added the feature label Jan 15, 2024

LukasJoss force-pushed the feat-PB-90-reduce_shortlink_location_popup_calls_tabs_version branch 2 times, most recently from 1e8bad8 to b4e2f29 Compare January 15, 2024 15:42

LukasJoss commented Jan 15, 2024

View reviewed changes

tests/cypress/tests-e2e/mouseposition.cy.js Show resolved Hide resolved

LukasJoss requested a review from ltshb January 15, 2024 16:07

ltshb reviewed Jan 17, 2024

View reviewed changes

LukasJoss closed this Jan 18, 2024

LukasJoss reopened this Jan 18, 2024

LukasJoss force-pushed the feat-PB-90-reduce_shortlink_location_popup_calls_tabs_version branch from 7614eb9 to abe8915 Compare January 18, 2024 15:57

LukasJoss requested a review from ltshb January 18, 2024 16:48

ltshb reviewed Jan 22, 2024

View reviewed changes

LukasJoss force-pushed the feat-PB-90-reduce_shortlink_location_popup_calls_tabs_version branch 3 times, most recently from 24a2af5 to 21106ca Compare January 24, 2024 16:34

LukasJoss requested a review from ltshb January 24, 2024 16:41

ltshb approved these changes Jan 24, 2024

View reviewed changes

LukasJoss force-pushed the feat-PB-90-reduce_shortlink_location_popup_calls_tabs_version branch from 7cb07ed to 5ac6711 Compare January 25, 2024 09:52

LukasJoss added 10 commits January 25, 2024 11:16

PB-90: Add collapsable button to location popup

cf545a6

PB-90: Add bootstrap padding and restrict shortlink updating

cf8d11b

PB-90: Clicking on share button it will now copy to clipboard

eec8520

PB-90: Share button changes text when used

d40efb3

PB-90: Add tooltip

31bdc70

PB-90: update e2e tests due to newly added button

5d37724

PB-90: Update tooltip and share button position

a63f1e9

PB-90: Update class names and variable types

ab6835c

PB-90: Test out looks with tab version

0fe5327

PB-90: update popup window

43cc2fe

LukasJoss added 15 commits January 25, 2024 11:16

PB-90: restructure code for location tabs

238d9bd

PB-90: Update passing of parameters for Popup Position

24c432f

PB-90: Update Popup Share Tab

fcb49f6

PB-90: Changed Popup share tab to properly update

ec0103d

PB-90: Add copying to clipboard by clicking on share tab

5d8ecca

PB-90: Add tooltip to share tab button

74cc7cb

PB-90: import modules to reuse code

cd09961

PB-90: Restructure code to make copy tab button work consistently

669257d

PB-90: remove duplicate qr-code

638ed2b

PB-90: remove qr codes form e2e test since since component tested els…

314f396

…ewhere

PB-90: Updated UI and now update sharlink on zoom

8f6ced5

PB-90: Update test case to check for social media share buttons to be…

f95013c

… present

PB-90: Pull share link update logic into parent component

f792ee9

PB-90: Update text content and size of share tab

f5f455d

PB-90: Update e2e test for share location popup

05661ac

LukasJoss force-pushed the feat-PB-90-reduce_shortlink_location_popup_calls_tabs_version branch from 5ac6711 to dc1876c Compare January 25, 2024 10:19

PB-90: Clean up code for PR

2605c07

LukasJoss force-pushed the feat-PB-90-reduce_shortlink_location_popup_calls_tabs_version branch from b285b68 to 2605c07 Compare January 25, 2024 12:28

LukasJoss merged commit 2b80df5 into develop Jan 25, 2024

LukasJoss deleted the feat-PB-90-reduce_shortlink_location_popup_calls_tabs_version branch January 25, 2024 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PB-90: Reduce shortlink location popup calls tabs version #603

PB-90: Reduce shortlink location popup calls tabs version #603

LukasJoss commented Jan 15, 2024 •

edited by github-actions bot

Loading

cypress bot commented Jan 15, 2024 •

edited

Loading

ltshb left a comment

LukasJoss commented Jan 18, 2024

LukasJoss commented Jan 18, 2024 •

edited

Loading

ltshb left a comment

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb Jan 22, 2024

ltshb left a comment

ltshb Jan 24, 2024

ltshb Jan 24, 2024

ltshb Jan 24, 2024

ltshb Jan 24, 2024

ltshb Jan 24, 2024

ltshb Jan 24, 2024

ltshb Jan 24, 2024

PB-90: Reduce shortlink location popup calls tabs version #603

PB-90: Reduce shortlink location popup calls tabs version #603

Conversation

LukasJoss commented Jan 15, 2024 • edited by github-actions bot Loading

cypress bot commented Jan 15, 2024 • edited Loading

Passing run #379 ↗︎

Review all test suite changes for PR #603 ↗︎

ltshb left a comment

Choose a reason for hiding this comment

LukasJoss commented Jan 18, 2024

LukasJoss commented Jan 18, 2024 • edited Loading

ltshb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltshb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasJoss commented Jan 15, 2024 •

edited by github-actions bot

Loading

cypress bot commented Jan 15, 2024 •

edited

Loading

LukasJoss commented Jan 18, 2024 •

edited

Loading