feat(gfi): migrate plugin#447
Conversation
58e73f6 to
f941649
Compare
bb7336c to
6a3129e
Compare
|
The usually helpful variables (map, stores, etc.) are auto-exported as global variables for use with the browser's devtools in snowbox.
acbf752 to
425cfdb
Compare
# Conflicts: # examples/snowbox/services.js
There was a problem hiding this comment.
- Please add an example to
iceberg; this maybe should include an example with a layer where the feature list is not being used - There should be no horizontal scrollbar if no features are available

- Some things are missing / quite different with the featureList; some parts are connections with the markers feature. This includes:
- When hovering an element in the feature list, the feature is highlighted in the map with the hover style
- When hovering an element in the map, the feature is highlighted in the feature list (previously green); when hovering a clustered feature, all features that are part of the cluster are highlighted
- If I select a feature in the map, it is selected in the feature list
- If I select a feature in the feature list, the corresponding marker gets the selected style; currently, a yellow dot is being displayed
- If I select a feature in the feature list, the map should be centered on that feature
- If a feature is not selectable because of the configured
isSelectablefunction, it is not being shown in the feature list
The list may not be complete, so please take a look at Meldemichel regarding the various things mentioned above.
I'll be taking a look at the components and stores once you've tackled these things.
| const features: Feature[] = [] | ||
| let feature: Feature | undefined | ||
|
|
||
| /* TODO: Format supposedly looks like this – is this a standard or arbitrary? |
There was a problem hiding this comment.
Are we able to answer this? Seems like a good point to maybe get rid of this comment
There was a problem hiding this comment.
Reading the current WMS spec, I could not find this format. However, this format may be specified in another way; or may at least be default for some implementations.
There was a problem hiding this comment.
Unfortunate! Please update the comment with your acquired knowledge.
| function getLayer(layerId: string) { | ||
| return map.value.getAllLayers().find((layer) => layer.get('id') === layerId) | ||
| } | ||
|
|
There was a problem hiding this comment.
There are 4 instances where we currently use coreStore.map.getLayers().getArray().find(...) where this action could also be used.
There was a problem hiding this comment.
Those 4 places are utils, and utils should not use stores (this would make unit tests harder, although we don't write them at the moment).
I also know that this rule is already violated in some utils, but I don't want to introduce more.
There was a problem hiding this comment.
Then that rather sounds like this action should become a utility function so it can be used across the project.
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
|
@oeninghe-dataport pls @ me once you've tackled all the things! |
For the hovering problems, this should be fixed with 9f25413. |
|
|
|
Feature list is not used, and you can choose between directSelect, directSelect + multiSelect, and coordinateSource from pins plugin. |
|
I think I've addressed all the points now. |
There was a problem hiding this comment.
- The feature list keeps telling me that "There are no features in the current view." if I initially opened it, then activated the MML layer and then opened the feature list again. It also doesn't update if the layer is active and the features take a second to update. I need to pan the map or zoom to then have an update.
- Only one feature is highlighted instead of all of the other features in a cluster. See the screenshot below how it is supposed to be implemented.

- The features should also be highlighted if I navigate the feature list with a keyboard.
- The horizontal scrollbar in
icebergseems like something that should not be there. - Like I mentioned in #447 (review), features that are not determined selectable should not be visible in the feature list

- A lot of the old implementation has not been removed yet. What is the reason behind that?
- An implementation like
setupZoomListenersseems to be missing. IIRC this was needed so the correct features stay selected when zooming in and out, which is no longer the case as seen below.

- Please add this plugin to
src/client.ts
| * Read or modify center coordinate of the map. | ||
| * | ||
| * @alpha | ||
| * @readonly | ||
| */ |
There was a problem hiding this comment.
I didn't find any instance where it is needed to update the center outside the core. Is this added in the anticipation of that change?
| /** | ||
| * Should the component be visible at all. | ||
| * | ||
| * @defaultValue `false` | ||
| */ |
There was a problem hiding this comment.
This parameter is also only relevant for layout: 'nineRegions'.
However, this may become an issue when using GFI as that plugin may very well be hidden to only display the information in an embedding system.
| * | ||
| * @defaultValue `'independent'` | ||
| */ | ||
| renderType?: 'independent' | 'iconMenu' | 'footer' |
There was a problem hiding this comment.
| renderType?: 'independent' | 'iconMenu' | 'footer' | |
| renderType?: 'independent' | 'iconMenu' |
'footer' can only be used by Attributions
| * | ||
| * @param map - Map | ||
| * @param feature - Feature to select | ||
| * @param centerOnFeature - Should the map center on the feature? |
There was a problem hiding this comment.
| * @param centerOnFeature - Should the map center on the feature? | |
| * @param centerOnFeature - Whether the map should center on the feature |
There was a problem hiding this comment.
Might be interesting to rename this to "useRefStore" and move it to the composables
|
|
||
| <style scoped> | ||
| #polar-card-gfi { | ||
| width: 12em; |
There was a problem hiding this comment.
Does KERN have a usable variable? Other than that, rem is to be preferred over em.
Also, this seems redundant if the plugin is added by the iconMenu
| id="polar-card-gfi" | ||
| :class="{ | ||
| standard: coreStore.layout === 'standard', | ||
| 'nine-regions': coreStore.layout === 'nineRegions', |
There was a problem hiding this comment.
Class does not seem to be used.
| <button | ||
| v-if="gfiStore.featureIndex > 0" | ||
| class="kern-btn kern-btn--block kern-btn--secondary" | ||
| @click="gfiStore.featureIndex--" | ||
| > | ||
| <span class="kern-icon kern-icon--arrow-back" aria-hidden="true" /> | ||
| <span class="kern-label"> | ||
| {{ $t(($) => $.switch.previous, { ns: 'gfi' }) }} | ||
| </span> | ||
| </button> | ||
| <button | ||
| v-if="gfiStore.featureIndex + 1 < gfiStore.features.length" | ||
| class="kern-btn kern-btn--block kern-btn--primary" | ||
| @click="gfiStore.featureIndex++" | ||
| > | ||
| <span class="kern-label"> | ||
| {{ $t(($) => $.switch.next, { ns: 'gfi' }) }} | ||
| </span> | ||
| <span class="kern-icon kern-icon--arrow-forward" aria-hidden="true" /> | ||
| </button> |
There was a problem hiding this comment.
I strongly prefer the previous switch buttons that had both directions displayed and only used the icons.
| <style scoped> | ||
| #polar-card-gfi { | ||
| width: 12em; | ||
| margin: var(--kern-metric-space-small); |
There was a problem hiding this comment.
This margin should not be added if the plugin is added by the iconMenu
| max-height: 80%; | ||
| overflow: auto; |
There was a problem hiding this comment.
These do nothing if the plugin is added by the iconMenu
| ] | ||
| : coordinateOrExtent | ||
|
|
||
| // TODO: Layer list needs better typing |
There was a problem hiding this comment.
Is this the same TODO as in lines 116 & 117?
I'd prefer having this as an issue in place of these TODOs
There was a problem hiding this comment.
I am not quite happy with this function. serialize is somewhat fitting, but I still am not able to directly identify what I receive without looking at the function or its TSDoc itself.
Also, isn't it clearer to just call new GeoJSON().writeFeatureObject(feature) in the two instances this util is used?
| export interface CustomHighlightStyle { | ||
| /** | ||
| * Object for defining the fill style. | ||
| * See [OpenLayers documentation](https://openlayers.org/en/latest/apidoc/module-ol_style_Fill-Fill.html) for full options. | ||
| */ | ||
| fill: Fill | ||
|
|
||
| /** | ||
| * Object for defining the stroke style. | ||
| * See [OpenLayers documentation](https://openlayers.org/en/latest/apidoc/module-ol_style_Stroke-Stroke.html) for full options. | ||
| */ | ||
| stroke: Stroke | ||
| } |
There was a problem hiding this comment.
The previous type was wrong, both should be optional as seen in the README.
There was a problem hiding this comment.
This is missing the _isMultiSelect handling, which should be needed when using multiSelect in conjunction with directSelect. How come this is no longer needed with pins though?
Also, I am not able to properly create a selection diff with this new implementation.
| if (gfiMainStore.configuration.directSelect) { | ||
| async function onMapClick({ coordinate, originalEvent }: MapBrowserEvent) { | ||
| await getFeatureInfo(coordinate as [number, number], { | ||
| toggleSelection: | ||
| navigator.userAgent.indexOf('Mac') !== -1 | ||
| ? originalEvent.metaKey | ||
| : originalEvent.ctrlKey, | ||
| }) | ||
| } | ||
|
|
||
| coreStore.map.on('click', onMapClick) | ||
| onScopeDispose(() => { | ||
| coreStore.map.un('click', onMapClick) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Please add the previous isDrawing condition. Otherwise, one would send off a gfi request when any of those previous conditions are true.
There was a problem hiding this comment.
As this function heavily relies on ol functionality, I'd prefer it to reside in src/plugins/gfi/stores/list.ts
| const overlay = new Overlay({ | ||
| positioning: 'bottom-center', | ||
| offset: [0, -5], | ||
| }) | ||
| coreStore.map.addOverlay(overlay) |
There was a problem hiding this comment.
Always adding the overlay is redundant. Please adhere to the previous implementation
| previous: 'Vorherige Seite', | ||
| wrapper: 'Seitenauswahl', | ||
| }, | ||
| } as const |
There was a problem hiding this comment.
| } as const | |
| export const resourcesEn = { | |
| pagination: { | |
| currentPage: 'Current page, page {{page}}', | |
| page: 'Page {{page}}', | |
| next: 'Next page', | |
| previous: 'VPrevious page', | |
| wrapper: 'Pagination', | |
| }, | |
| } as const |
| locales, | ||
| storeModule: useGfiStore as PolarPluginStore, | ||
| options, | ||
| icon: 'kern-icon--map-pin-heart', |
There was a problem hiding this comment.
Not really sure if this should be added here by default.
| (async () => { | ||
| gfiStore.hoveredFeatures = {} | ||
| await nextTick() | ||
| gfiStore.selectedFeatures = { [layerId]: [feature] } | ||
| })() | ||
| " |
There was a problem hiding this comment.
The previous implementation with setOlFeatureInformation should be preferred over this.
| <h2 class="kern-heading-medium"> | ||
| {{ $t(($) => $.list.header, { ns: 'gfi' }) }} | ||
| </h2> |
| <template v-else-if="gfiStore.configuration.featureList"> | ||
| <GfiFeatureList /> | ||
| </template> |
There was a problem hiding this comment.
| <template v-else-if="gfiStore.configuration.featureList"> | |
| <GfiFeatureList /> | |
| </template> | |
| <GfiFeatureList v-else-if="gfiStore.configuration.featureList" /> |
| <p | ||
| v-if="gfiStore.listFlatFeatures.length === 0" | ||
| class="kern-body kern-body--small polar-plugin-gfi-list-empty-view" | ||
| > | ||
| {{ $t(($) => $.list.emptyView, { ns: 'gfi' }) }} | ||
| </p> |
There was a problem hiding this comment.
Based on https://www.kern-ux.de/komponenten/card, this should be wrapped in a <section> with class="kern-card__body"
| <h2 class="kern-heading-medium"> | ||
| {{ $t(($) => $.list.header, { ns: 'gfi' }) }} | ||
| </h2> |
There was a problem hiding this comment.
Based on https://www.kern-ux.de/komponenten/card, this should be wrapped in a <header> with class="kern-card__header"
| <h2 class="kern-title kern-title--small"> | ||
| {{ gfiStore.listGetText(feature, 'title') }} | ||
| </h2> |
There was a problem hiding this comment.
| <h2 class="kern-title kern-title--small"> | |
| {{ gfiStore.listGetText(feature, 'title') }} | |
| </h2> | |
| <h3 class="kern-title kern-title--small"> | |
| {{ gfiStore.listGetText(feature, 'title') }} | |
| </h3> |
| {{ gfiStore.listGetText(feature, 'subtitle') }} | ||
| <br /> |
| @click=" | ||
| (async () => { | ||
| gfiStore.hoveredFeatures = {} | ||
| await nextTick() | ||
| gfiStore.selectedFeatures = { [layerId]: [feature] } | ||
| })() | ||
| " | ||
| @mouseenter="gfiStore.hoveredFeatures = { [layerId]: [feature] }" | ||
| @mouseleave="gfiStore.hoveredFeatures = {}" |
There was a problem hiding this comment.
This logic belongs to the store
| text-wrap: wrap; | ||
| } | ||
|
|
||
| section { |
There was a problem hiding this comment.
Can kern-card__body handle a lot of this?
|
|
||
| export const gfiFailedSymbol = Symbol('POLAR gfi call failed') |
There was a problem hiding this comment.
| export const gfiFailedSymbol = Symbol('POLAR gfi call failed') |
Not used
| /** | ||
| * (WMS-only) | ||
| * Some WMS services may return features close to the clicked position. | ||
| * By setting the value `clickPosition`, only features that intersect the clicked position are shown to the user. | ||
| * | ||
| * @defaultValue Show all features | ||
| */ | ||
| filterBy?: 'clickPosition' | ||
|
|
||
| /** | ||
| * (WMS-only) | ||
| * If the `infoFormat` is not set to `'application/geojson'`´, this can be configured to be the known file format of the response. | ||
| * If not given, the format is parsed from the response data. | ||
| */ | ||
| format?: 'GML' | 'GML2' | 'GML3' | 'GML32' | 'text' |
There was a problem hiding this comment.
As there are only for WMS-layers, can we list them separately? Same would go for showTooltip for Vectorlayers.
| /** parameter specification for request method */ | ||
| export interface RequestGfiParameters { | ||
| coordinateOrExtent: [number, number] | [number, number, number, number] | ||
| layer: BaseLayer | ||
| layerConfiguration: GfiLayerConfiguration | ||
|
|
||
| /** rawLayerList entry, see https://bitbucket.org/geowerkstatt-hamburg/masterportal/src/dev/doc/services.json.md */ | ||
| layerSpecification: Record<string, unknown> | ||
|
|
||
| map: Map | ||
|
|
||
| /** defaults to bboxDot (get from minimal coordinate bbox) */ | ||
| mode?: 'bboxDot' | 'intersects' | ||
| } | ||
|
|
||
| export interface RequestGfiWmsParameters { | ||
| coordinate: [number, number] | ||
| layer: TileLayer<TileWMS> | ImageLayer<ImageWMS> | ||
| layerConfiguration: RequestGfiParameters['layerConfiguration'] | ||
| layerSpecification: RequestGfiParameters['layerSpecification'] | ||
| map: RequestGfiParameters['map'] | ||
| } |
There was a problem hiding this comment.
I'm not sure whether these should be moved to a different file to not include them in the documentation. What's your take?
| */ | ||
| export interface GfiLayerConfiguration { | ||
| /** | ||
| * Property of the features of a service having an url usable to trigger a download of features as a document. |
There was a problem hiding this comment.
| * Property of the features of a service having an url usable to trigger a download of features as a document. | |
| * Property of the features of a service having a url usable to trigger a download of features as a document. |
|
|
||
| /** | ||
| * (WMS-only) | ||
| * If the `infoFormat` is not set to `'application/geojson'`´, this can be configured to be the known file format of the response. |
There was a problem hiding this comment.
It needs to be specified that the infoFormat is set on the layer in the service registry.
| featureList?: FeatureList | ||
|
|
||
| /** | ||
| * Limits the viewable GFIs per layer by this number. |
There was a problem hiding this comment.
| * Limits the viewable GFIs per layer by this number. | |
| * Limits the viewable results per layer by this number. |
| * Limits the viewable GFIs per layer by this number. | ||
| * The first n elements are chosen arbitrarily. | ||
| * Useful if you e.g. just want one result, or to limit an endless stream of returns to e.g. 10. | ||
| * Infinite by default. |
There was a problem hiding this comment.
@defaultValue Infinity should be preferred
|
|
||
| /** | ||
| * Method of calculating which feature has been chosen by the user. | ||
| * `bboxDot` utilizes the `bbox`-url parameter using the clicked coordinate while `intersects` uses a `Filter` to calculate the intersected features. |
There was a problem hiding this comment.
| * `bboxDot` utilizes the `bbox`-url parameter using the clicked coordinate while `intersects` uses a `Filter` to calculate the intersected features. | |
| * `bboxDot` utilizes the `bbox`-url parameter using the clicked coordinate | |
| * while `intersects` uses a `Filter` to calculate the intersected features. |
| * Method of calculating which feature has been chosen by the user. | ||
| * `bboxDot` utilizes the `bbox`-url parameter using the clicked coordinate while `intersects` uses a `Filter` to calculate the intersected features. | ||
| * Layers can have their own `gfiMode` parameter which would override this global mode. | ||
| * To apply this, add the desired value to the parameter in the `mapConfiguration`. |
There was a problem hiding this comment.
| * To apply this, add the desired value to the parameter in the `mapConfiguration`. | |
| * To apply this, add the desired value to the parameter in the {@link MapConfiguration | `mapConfiguration`}. |
| * If set to `'box'`, the selection will be done in a box. | ||
| * If set to `'circle'`, the selection will be done in a circle. | ||
| * | ||
| * Similar to `directSelect`, features can be added and removed by selection / unselecting them. |
There was a problem hiding this comment.
| * Similar to `directSelect`, features can be added and removed by selection / unselecting them. | |
| * Similar to {@link GfiPluginOptions.directSelect | `directSelect`}, | |
| * features can be added and removed by selection / unselecting them. |




Summary
Migrate the GFI plugin.
Instructions for local reproduction and review
Additional hints
requestGfi*were migrated as-is and do not need to be reviewed therefore.Relevant tickets, issues, et cetera
Closes #368