GSLUX-756: Display feature info in info panel #171
Conversation
tkohr
force-pushed
the
GSLUX-756-feature-info
branch
from
c31406e
to
2ecf13b
Compare
|
GitHub Pages links: * Luxembourg-geoportail: https://geoportail-luxembourg.github.io/luxembourg-geoportail/GSLUX-756-feature-info/ |
tkohr
force-pushed
the
GSLUX-756-feature-info
branch
from
9dec5ad
to
6d436a1
Compare
note: with z-20 it appears over sidepanel on mobile
aka isQuerying, hiddenContent, infoOpen in v3
to distinguish pan, longpress and featureInfo from feature draw/edit
which should already be handled by draw/edit logic
make featureInfoService a singleton at the same time
to display cursor pointer on layers
…dio link and ordering fix panel overflow, clean dom, add util to sanitize url along the way
also use DefaultTemplate as fallback for templates not yet migrated
does not work well with multiple profiles yet
from showProfile.active to has_profile
tkohr
force-pushed
the
GSLUX-756-feature-info
branch
from
6d436a1
to
8f304a6
Compare
| </button></a | ||
| > | ||
| <span | ||
| v-if="isLink(entry['value']) && entry['key'] == 'f_AudioURL'" |
| " | ||
| :src="`${getTrustedUrl(entry['value'])}`" | ||
| title="water level graph" | ||
| ></iframe> |
There was a problem hiding this comment.
Can we have somewhere (in the doc, or in code comment) the corresponding layers to test each cases please?
| <profile-feature-info :feature="feature" /> | ||
| </div> | ||
| <div v-if="layers.has_profile"> | ||
| <a |
There was a problem hiding this comment.
Not very important bu this would be button instead of a link I guess, as there is no href (for accessibility?)
| import { defineProps } from 'vue' | ||
| defineProps({ | ||
| layers: { | ||
| type: Object, |
There was a problem hiding this comment.
I am expecting an array of layers here, or am i missing something? Can we have a better typing than Object?
| @@ -0,0 +1,152 @@ | |||
| import { useTranslation } from 'i18next-vue' | |||
| import { FeatureJSON } from '../../../services/info/feature-info.model' | |||
| }[] { | ||
| return Object.entries(attributes) | ||
| .filter(([key]) => key !== 'showProfile') | ||
| .map(([key, value]) => ({ key: prefix + key, value })) |
There was a problem hiding this comment.
I would have done the sorting here (instead of inside the template)
| ) | ||
| } | ||
|
|
||
| export function isEmpty(value: string | undefined | null): boolean { |
There was a problem hiding this comment.
isEmptyString maybe?
| ok = true | ||
| } | ||
|
|
||
| return ok |
There was a problem hiding this comment.
If you have time it oculd be great to refactor this huge test into small ones with named cases (eg. storage-rules-version-heper.ts)
| return sanitizeUrl(urlEn ?? urlFr) | ||
| case 'lb': | ||
| return sanitizeUrl(urlLb ?? urlFr) | ||
| default: |
There was a problem hiding this comment.
maybe regroup case 'fr' and default
| @@ -15,6 +15,7 @@ import FullscreenControl from '../map-controls/fullscreen-control.vue' | |||
| import ZoomControl from '../map-controls/zoom-control.vue' | |||
| import ZoomToExtentControl from '../map-controls/zoom-to-extent-control.vue' | |||
| import useDraw from '@/composables/draw/draw.composable' | |||
| import useFeatureInfo from '../../composables/info/feature-info.composable' | |||
| (featureMatch && | ||
| (layersOpen.value || myMapsOpen.value) && | ||
| drawnFeatures.value.length > 0) || | ||
| (featureMatch && layers.value.length === 0) |
There was a problem hiding this comment.
both conditions have featureMatch, right? can be simplified
| } | ||
|
|
||
| const tempTime = new Date().getTime() | ||
| if (tempTime - pointerUpTime.value <= 499) { |
There was a problem hiding this comment.
I think this one is throttle? you can find one in the utils.ts
| if (timeoutId.value !== null) { | ||
| clearTimeout(timeoutId.value) | ||
| } | ||
| })() |
There was a problem hiding this comment.
any reason why this is a self called function? (same question for event above, is it necessary?)
| if (fid.value) { | ||
| ;(async () => { | ||
| await getFeatureInfoById(fid.value as string) | ||
| })() |
There was a problem hiding this comment.
any reason why using this synthax?
| try { | ||
| if (layersList.length > 0) { | ||
| setLoading(true) | ||
| map.getViewport().style.cursor = 'wait' |
There was a problem hiding this comment.
is the style cursor going back to default point in this function? I don't see where. Maybe the cursor style can be handled in a watch on loading ref.
| setInfoPanelHidden(true) | ||
| } | ||
|
|
||
| const node = findById(splittedFid[0]) |
There was a problem hiding this comment.
can splittedFid[0] be declared as a const? like const fId = splittedFid[0] if i understood well
| reset() | ||
| } | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
it looks like you can merge these two try/catch ? because you are doing the same reset() and nothing more in both cases
| ] | ||
|
|
||
| setLoading(true) | ||
| map.getViewport().style.cursor = 'wait' |
There was a problem hiding this comment.
it looks like this can be done in a watcher
|
|
||
| const url = new URL(INFO_SERVICE_URL) | ||
| Object.keys(params).forEach(key => | ||
| url.searchParams.append(key, (params as Record<string, any>)[key]) |
There was a problem hiding this comment.
you can use existing fetchApi() (api.service.ts) you won't need to parse the params object like this.
| } | ||
| } | ||
|
|
||
| async function singleclickEvent(evt: MapBrowserEvent<any>): Promise<void> { |
There was a problem hiding this comment.
if i read the function name i don't know it will trigger an api call, either make it clear it the naming that they will be a call, or extract the api call into a new function (eg. existing services in src/services/api/ folder)
| lastHighlightedFeatures.value, | ||
| false | ||
| ) | ||
| reset() |
There was a problem hiding this comment.
I think instead of this sequential handling you could have use more watchers, eg:
on click event => trigger call api and set the feature data ref
have a watcher on feature data ref => on change, trigger reset if the data is invalid, or trigger display if valid
| item['layerLabel'] = layerLabel[item.layer] | ||
| let found = false | ||
| for (let iLayer = 0; iLayer < responses.value.length; iLayer++) { | ||
| if (responses.value[iLayer].layer == item.layer) { |
| data.forEach(item => { | ||
| item['layerLabel'] = layerLabel[item.layer] | ||
| let found = false | ||
| for (let iLayer = 0; iLayer < responses.value.length; iLayer++) { |
There was a problem hiding this comment.
you don't like array functions anymore? ^^ I guess it is a copy past from v3 ;)
| hitTolerance: 5, | ||
| } | ||
| ) | ||
| return selected.length > 0 ? true : false |
There was a problem hiding this comment.
| return selected.length > 0 ? true : false | |
| return selected.length > 0 |
| onUnmounted(() => { | ||
| clearContent() | ||
| featureInfoLayerService.clearFeatures() |
There was a problem hiding this comment.
This clearFeatures is done 3 times in the composable, maybe it would be better to keep this one in the composable also (you can add onunmouted in composables).
Also, why not doing a reset() here?
| ] | ||
|
|
||
| const geometryType = feature.getGeometry()?.getType() | ||
| return geometryType == 'Point' || geometryType == 'MultiPoint' |
| if (fit && extent) { | ||
| const fitOptions: FitOptions = { | ||
| size: this.map.getSize() as Size, | ||
| maxZoom: 17, |
There was a problem hiding this comment.
There is a maxzoom in map store that can be set by the config theme, I don't know if this maxZoom in store should be taken into account. (Can be done later)
|
Thanks for this huge work! Tested on github pages, working great, some minor things i have noticed:
|
| )" | ||
| :key="entry.key" | ||
| > | ||
| <span |
There was a problem hiding this comment.
there is already a div wrapper above, i think you can get rid of this span (plus it is more logic to seperate lines with a div than with a span, not very important but this is html semantic)
JIRA issue
https://jira.camptocamp.com/browse/GSLUX-756
Description
The goal of this PR is to set a base for the feature info. It now displays the feature info of the default template.
Remaining to-dos for separate PRs remain:
QueryControllerinfo.html)Permalink with all layers to test
default-template.html: http://localhost:5173/theme/main?version=3&lang=fr&X=702429&Y=6396653&zoom=16&rotation=0&features=&layers=655-2842-808-1713-1714-269-302-1813&opacities=1-0-1-1-1-1-1-1&time=--------------&bgLayer=basemap_2015_global