Gslux 755 location info #175

mki-c2c · 2024-11-22T09:50:55Z

JIRA issue

https://jira.camptocamp.com/browse/GSLUX-XXX

Description

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

Screenshots

(only if the final render is different)

github-actions · 2024-11-22T09:51:59Z

GitHub Pages links: * Luxembourg-geoportail: https://geoportail-luxembourg.github.io/luxembourg-geoportail/GSLUX-755-location_info/

AlitaBernachot · 2024-11-29T11:20:34Z

cypress/e2e/footer-bar.cy.ts

@@ -31,7 +31,7 @@ describe('Footer bar', () => {
      cy.get('[data-cy="infoOpenClose"]').find('button').click()
      cy.get('[data-cy="infoPanel"]').should('exist')
      cy.get('button[data-cy="drawButton"]').click()
-      cy.get('[data-cy="infoPanel"]').should('not.exist')
+      cy.get('[data-cy="infoPanel"]').should('be.hidden')


Any reason why hiding the panel with css instead of removing it from the dom?

AlitaBernachot · 2024-11-29T11:24:53Z

src/components/info/info-panel.vue

-        </ul>
-      </div>
+      <template v-if="map">
+        <div v-show="locationInfo" class="absolute">


any reason why using v-if l.37 and v-show l.38? could this be done using only one if? (and use v-else l.43)

v-show is needed so that the streetview component remains persistant, so that the dom reference in google code remains valid
v-if removes thecomponent from the dom

Ok understood, I hope this won't break the v3 panel because I remember facing some issues when the v3 was just hidden.But maybe it won't be the case for your Streetview panel.

AlitaBernachot · 2024-11-29T11:27:58Z

src/components/info/location-info.vue

+
+const map = useMap().getOlMap()
+
+const shortUrl = ref()


it would be great to add some typing here, I don't even understand how this can pass the linter...

AlitaBernachot · 2024-11-29T11:35:32Z

src/components/info/location-info.vue

+    clickCoordinateLuref.value = info.clickCoordinateLuref
+    formatted_coordinates.value = info.formatted_coordinates
+    elevation.value = await getElevation(info.clickCoordinateLuref)
+    address.value = await getNearestAddress(info.clickCoordinateLuref)


You are doing 3 await sequentially, this means the user is going to wait a bit more at each request. Please use parallele loading instead (eg. Promise.allSettled).

yes, good idea, the awaits came together after refactoring,and I did not notice the possibility to regroup

AlitaBernachot · 2024-11-29T11:38:26Z

src/components/info/location-info.vue

+const elevation = ref()
+const address = ref()
+const clickCoordinateLuref = ref()
+const formatted_coordinates = ref({})


We do use camel case in our code since the beginning of the project.

yep,
do you think it's possible to enforce camelCase in the linter if it is a project standard?

yes, "camelcase": "on" (I guess, did not test)

https://eslint.org/docs/latest/rules/camelcase#rule-details

AlitaBernachot · 2024-11-29T11:39:39Z

src/components/info/location-info.vue

+
+const isRapportForageVirtuelAvailable = computed(() => {
+  const userRole = 'ACT'
+  return userRole === 'ACT'


Maybe add a comment here? because I don't understand the utility of this (maybe because the PR isn't finished yet?)

sorry, still a stub (I should have marked it)

FYI if you need user's role it is available in the auth service.

or user-manager store

AlitaBernachot · 2024-11-29T11:40:45Z

src/components/info/location-info.vue

+const getLidarUrl = () => 'bla'
+const cyclomediaUrl = computed(() =>
+  clickCoordinateLuref.value
+    ? `http://streetsmart.cyclomedia.com/streetsmart?q=${clickCoordinateLuref.value[0]};${clickCoordinateLuref.value[1]}`


Move http://streetsmart.cyclomedia.com/streetsmart to a const in .env

AlitaBernachot · 2024-11-29T11:41:18Z

src/components/info/location-info.vue

+const isCyclomediaAvailable = computed(() => true)
+const isImagesObliquesAvailable = computed(() => true)
+
+const getLidarUrl = () => 'bla'


I hope this won't stay as is.

AlitaBernachot · 2024-11-29T11:44:06Z

src/components/info/location-info.vue

+const addRoutePoint = () => '// TODO: add point to route'
+
+const open = ref(true)
+const { isStreetviewActive } = storeToRefs(useInfoStore())


useInfoStore() is also called l. 18, instead declare a new const infoStore = useInfoStore() and use this new const.

thanks, I'll do a one-line destructioning

AlitaBernachot · 2024-11-29T11:45:11Z

src/components/info/location-info.vue

+
+const open = ref(true)
+const { isStreetviewActive } = storeToRefs(useInfoStore())
+const toggleStreetview = () => {


Suggested change

const toggleStreetview = () => {

function toggleStreetview () {

Any reason why this is declared as a const?

is there any difference ?

Code consistency?

AlitaBernachot · 2024-11-29T11:46:12Z

src/components/info/location-info.vue

+
+function downloadRapportForageVirtuel() {
+  downloadingRepport.value = true
+  setTimeout(() => (downloadingRepport.value = false), 2000)


maybe add a comment here? it looks like it is unfinished

AlitaBernachot · 2024-11-29T11:47:53Z

src/components/info/location-info.vue

+<template>
+  <div class="flex flex-row">
+    <div class="grow-[2] flex flex-col content-end">
+      <div class="grow"></div>


don't add empty dom node just for display.

I need a spacer for flex

don't add empty dom node just for display.

justify-end works also

AlitaBernachot · 2024-11-29T11:48:43Z

src/components/info/location-info.vue

+      <h3 class="text-3xl text-white">
+        {{ t('Short Url', { ns: 'client' }) }}
+      </h3>
+      <input


input should have a name or id attribute to be html valid (and accessibility compliance).

tricky one, thanks

added: aria-labelledby="short_url_title"

no name needed since input is not part of a form

AlitaBernachot · 2024-11-29T11:52:20Z

src/components/info/location-info.vue

+          <td>{{ coords }}</td>
+        </tr>
+        <tr>
+          <th style="text-align: left">{{ t('Elevation') }}</th>


use tailwind class

forgot to remove (handled by CSS)

AlitaBernachot · 2024-11-29T11:52:27Z

src/components/info/location-info.vue

+          <td>{{ elevation }}</td>
+        </tr>
+        <tr>
+          <th style="text-align: left">{{ t('Adresse la plus proche') }}</th>


use tailwind class

forgot to remove (handled by CSS)

AlitaBernachot · 2024-11-29T11:52:37Z

src/components/info/location-info.vue

+          <td>{{ address?.formattedAddress }}</td>
+        </tr>
+        <tr>
+          <th style="text-align: left">{{ t('Distance approximative') }}</th>


use tailwind class

forgot to remove (handled by CSS)

AlitaBernachot · 2024-11-29T11:53:30Z

src/components/info/location-info.vue

+    <img class="mx-5 mt-5" v-if="qrUrl" :src="qrUrl" />
+  </div>
+  <div>
+    <div class="mt-5">


looks like you can get rid of this dev and add the "mt-5" class to the h3 below.

AlitaBernachot · 2024-11-29T11:54:14Z

src/components/info/location-info.vue

+    </table>
+  </div>
+  <div>
+    <div v-if="isRapportForageVirtuelAvailable">


look like this div is not useful and the condition can be attached to the button below

AlitaBernachot · 2024-11-29T11:56:10Z

src/components/info/location-info.vue

+      <a
+        v-if="isInBoxOfLidar"
+        class="lux-btn whitespace-nowrap"
+        :href="getLidarUrl()"


why is this one getter whereas the others are refs?

AlitaBernachot · 2024-11-29T11:59:17Z

src/components/info/street-view.vue

+    class="h-[500px]"
+    v-show="isStreetviewActive && !noDataAtLocation"
+  >
+    Streetview Container


Is this a title? any translation for that?

AlitaBernachot · 2024-11-29T12:00:30Z

src/components/info/street-view.vue

+    Streetview Container
+  </div>
+  <div
+    class="grid before:content-streetview before:col-start-1 before:row-start-1"


Why is this a grid? you only display a text message.

modern equivalent of position: absolute
https://dev.to/nicm42/using-css-grid-to-put-elements-on-top-of-each-other-44ei

AlitaBernachot · 2024-11-29T12:01:05Z

src/components/side-panel/side-panel.vue

@@ -93,7 +93,7 @@ watch(infoOpen, infoOpen => {
  </div>

  <!-- Info panel -->
-  <div v-if="infoOpen" class="w-full md:w-80 bg-secondary z-10">
+  <div v-show="infoOpen" class="w-full md:w-80 bg-secondary z-10">


same as first comment above, any reason why you are hiding the panel instead of remove it from the dom like the others?

streetview needs to be persistent

AlitaBernachot · 2024-11-29T12:02:04Z

src/composables/map/location-info.composable.ts

+    if (doHide) {
+      infoFeatureLayer.getSource()?.clear()
+    } else {
+      if (locationInfo.value) {


use else if

AlitaBernachot · 2024-11-29T12:04:11Z

src/composables/map/location-info.composable.ts

+    if (location && !hidePointer.value) {
+      infoOpen.value = true
+      const feature = new Feature(new Point(location))
+      infoFeatureLayer.getSource()?.addFeature(feature)


This watcher is very similar to the previous one, maybe they can be combined into one.
Or at least a bit of refactor, move

const feature = new Feature(new Point(location)) infoFeatureLayer.getSource()?.addFeature(feature)

into a new function eg. createNewPoint()

good idea, thanks

AlitaBernachot · 2024-11-29T12:06:51Z

src/composables/map/location-info.composable.ts

+    return map.getEventCoordinate(event.originalEvent)
+  }
+
+  const handleClick = function (event: MapBrowserEvent<PointerEvent>) {


Any reason why declaring function 3 different ways in this file? (getClickCoordinate, handleClick, setInfoStyle)

typescript's fault ;-)

AlitaBernachot · 2024-11-29T12:07:50Z

src/composables/map/location-info.composable.ts

+    if (event.originalEvent.button === 2) {
+      // if right mouse click
+      locationInfo.value = getClickCoordinate(event)
+    } else if (event.originalEvent.pointerType == 'touch') {


AlitaBernachot · 2024-11-29T12:08:37Z

src/composables/map/location-info.composable.ts

+    }
+  }
+
+  listen(map, 'pointerup', (event: any) => {


don't use any, you have found the right typing below event as MapBrowserEvent<PointerEvent>

it's a MapBrowserEvent<MouseEvent>, unfortunaltely incompatible

incompatible with what? don't use any, please. You can remove it, TS will consider it as a BaseEvent.

Thanks, I had not tried simply removing it :-(

AlitaBernachot · 2024-11-29T12:09:31Z

src/composables/map/location-info.composable.ts

+  listen(map, 'pointerup', (event: any) => {
+    if (
+      startPixel &&
+      (event as MapBrowserEvent<PointerEvent>).originalEvent.button === 0


Move the typing event as MapBrowserEvent<PointerEvent> up l. 85 to replace the any

does not work, above is MapBrowserEvent<MouseEvent> so it needs to be cast to any however

incompatible with what? don't use any, please. You can remove it, TS will consider it as a BaseEvent.

yes, just removing it works, thanks!

(I had tried all sort of combinations, BaseEvent was not accepted, the signature wanted Event | BaseEvent, but I could not figure out where to find "Event" )

…liable

comply with assumptions in feature info tests

mki-c2c · 2024-12-13T11:50:02Z

hello @tkohr
I did the rebase wrt the feature info branch. could you please check the latest commits if you think this is OK with your code ?

I would like to do some more harmonization, what do you think of it ?:

move locationInfo and streetview composables into composables/info
use only one info store with both location and feature states => rename info store to location-info store
move locationInfo e2e test into cypress/e2e/info

…value (https://docs.cypress.io/api/commands/should#Function)

tkohr · 2024-12-13T15:03:19Z

src/components/info/info-panel.vue

 onUnmounted(() => {
  clearContent()
 })


If we really need the v-show above this component, we could get rid of the onUnmounted I guess ?

you're right.

I could not set up the correct link to street view without the component being persistent

tkohr · 2024-12-13T15:03:29Z

hello @tkohr I did the rebase wrt the feature info branch. could you please check the latest commits if you think this is OK with your code ?

Thanks for the rebase @mki-c2c ! Looks nice to see the right click and left click work together. Didn't notice any issues with the feature info part.

I would like to do some more harmonization, what do you think of it ?:
* [ ]  move locationInfo and streetview composables into composables/info

Good idea!

* [ ]  use only one info store with both location and feature states

What do you think of creating an info folder within the stores, renaming the info.store to location-info.store and moving location-info.store and feature-info.store to the same folder? I feel like keeping both separated in different stores, keeps things clearer.

* [ ]  move locationInfo e2e test into cypress/e2e/info

Good idea!

mki-c2c force-pushed the GSLUX-755-location_info branch from 16b4f94 to 129ffb0 Compare November 27, 2024 13:50

AlitaBernachot reviewed Nov 29, 2024

View reviewed changes

mki-c2c added 15 commits December 12, 2024 09:50

add correct logics for buttons

28b75b0

provide support for routing in info panel

9f4f92a

deactivate location info when the info panel is closed

a66c0dc

export necessary vars for info integration in v3

84cd76b

integrate v4 location info component

4355e2f

fix typing

748df56

fix unit tests

cc13f23

add e2e test

8bf36ff

review comments, import typing for google maps API

984ac02

add docstring

a857a73

refactoring of url download and constants for CRS

f3ec572

improve DOM

1c6354f

use fixed API responses to have predictable test results

2dbe417

synchronize postrender for streetview tests

e943a0b

integrate location info and feature info

e5ec30d

mki-c2c force-pushed the GSLUX-755-location_info branch from 11b0349 to e5ec30d Compare December 12, 2024 11:12

mki-c2c added 3 commits December 13, 2024 12:21

tune assertions to fix timing in order to make location info tests re…

d5c8ad1

…liable

fix permalink

a814426

comply with assumptions in feature info tests

fix e2e test

e280ba4

mki-c2c added 2 commits December 13, 2024 13:09

use arrow function in should will make cypress wait for the expected …

b2efe06

…value (https://docs.cypress.io/api/commands/should#Function)

fix test

525c5e6

tkohr reviewed Dec 13, 2024

View reviewed changes

mki-c2c added 5 commits December 13, 2024 16:21

fix legends test

2c3f0d8

refactor info features

af2d822

explicit wait for legend fixtures

1954e75

remove unnecessary unmount hook (review comments)

333a240

explicit wait for profile API calls

ce754fa

mki-c2c merged commit aea4b4b into main Dec 17, 2024
2 checks passed

	const toggleStreetview = () => {
	function toggleStreetview () {

Gslux 755 location info #175

Gslux 755 location info #175

Conversation

mki-c2c commented Nov 22, 2024

JIRA issue

Description

Screenshots

github-actions bot commented Nov 22, 2024

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

AlitaBernachot Nov 29, 2024 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot Nov 29, 2024 • edited Loading

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

mki-c2c Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mki-c2c commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkohr commented Dec 13, 2024

AlitaBernachot Nov 29, 2024 •

edited

Loading

AlitaBernachot Nov 29, 2024 •

edited

Loading

AlitaBernachot Nov 29, 2024 •

edited

Loading

mki-c2c Dec 1, 2024 •

edited

Loading

mki-c2c commented Dec 13, 2024 •

edited

Loading