[Basemap-switcher] Use a image based basemap switcher #362
Conversation
🦋 Changeset detectedLatest commit: 298fc6f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -116,30 +152,60 @@ export const BasemapSwitcher: FC<BasemapSwitcherProps> = (props) => { | |||
| return ( | |||
| <Box {...containerProps}> | |||
| {map ? ( | |||
| <Select<SelectOption> | |||
There was a problem hiding this comment.
I think it better do have a separate component ImageBaseampSwitcher in the same package.
That would make the code more comprehensible and avoid the problem that some of the component's props only apply to either default or image mode.
| console.error("Failed to find a layer"); | ||
| return; | ||
| } | ||
| map?.layers.activateBaseLayer(layer.id); |
There was a problem hiding this comment.
The actual logic to switch the base map should be inside the basemap switcher.
I think the (Image)BasemapSwitcher should only get the configuration (e.g. which thumbnail for which basemap) through the component properties. The state (selection) and the actual code to switch the basemap belong to the component itself.
There was a problem hiding this comment.
In a previous talk, we wanted to combine them, but I think it would make more sense to add the ImageBaseampSwitcher as a separate component.
The switch-logic is intentionally outside, because in our case not all basemaps should be available. There is a map-model logic, that says: if appstate A: then basemaps B1 & B2 are selectable, but not B3, else if state [...]. This opens the question if this is maybe to spceial for the general use case, or something that we want to include. If it's too special i would agree to hide the switching logic to the component itself
There was a problem hiding this comment.
Perhaps a simple basemapFilter predicate (return false for basemaps that should be excluded) would be sufficient?
| const imageBasemapSwitcher = useMemo(() => { | ||
| return { | ||
| selectedImageLabel: imageLabel, | ||
| choosableImageLabel: currentSelection |
There was a problem hiding this comment.
rename to selectableImageLabel?
|
Thanks for creating this PR. I just had a look on your updates. Generally, I think this is quite nice and should be part of the base packages.
[0] https://github.com/open-pioneer/trails-openlayers-base-packages/blob/main/src/samples/showcase/showcase-app/MapConfigProviderImpl.ts#L110 |
…ntern handling of baselayer, config of thumbnails
…ntern handling of baselayer, config of thumbnails
|
Thanks @arnevogt for your feedback. I have worked on the open points and i think we have a more common solutions.
I'm still open for any improvements :) |
Just FYI Sharing the hook in a shared file within the package could still be done, of course. But it's not deserving a real API. |
| * Alternatively, the image and label can be set in the layer attributes. | ||
| * If no images are set, the default image will be used. | ||
| */ | ||
| imageMap?: Map<string, BaselayerImageBasemapSwitcherProps>; |
There was a problem hiding this comment.
using Map<> in React is not ideal:
Maps are mutable, if someone updates the Map (set, delete) from the outside this will not trigger a re-render of the component.
I suggest using a immutable type like Record, Array or a custom (immutable) object.
| const { src, label, callback, width, height } = props; | ||
| return ( | ||
| <MenuItem onClick={() => callback()}> | ||
| <Tooltip label={label}> |
There was a problem hiding this comment.
Thumbs up if aria-labels are supported
| ?.imageBasemapSwitcher as BaselayerImageBasemapSwitcherProps; | ||
| const parameter = imageMap?.get(selectedLayer?.id || NO_BASEMAP_ID); | ||
| const image = | ||
| basemap?.image || parameter?.image || (!selectedLayer ? emptyImage : defaultImage); |
There was a problem hiding this comment.
I think it would be more intuitive if parameter.image has precedence.
Typically (e.g. with CSS), properties directly on the component have precedents over definitions elsewhere.
| const parameter = imageMap?.get(selectedLayer?.id || NO_BASEMAP_ID); | ||
| const image = | ||
| basemap?.image || parameter?.image || (!selectedLayer ? emptyImage : defaultImage); | ||
| const label = |
There was a problem hiding this comment.
regarding precedence see comment above
| function useBaseLayers(mapModel: MapModel | undefined): Layer[] { | ||
| // Caches potentially expensive layers arrays. | ||
| // Not sure if this is a good idea, but getSnapshot() should always be fast. | ||
| // If this is a no-go, make getAllLayers() fast instead. | ||
| const baseLayers = useRef<Layer[] | undefined>(); | ||
| const subscribe = useCallback( | ||
| (cb: () => void) => { | ||
| // Reset cache when (re-) subscribing | ||
| baseLayers.current = undefined; | ||
|
|
||
| if (!mapModel) { | ||
| return () => undefined; | ||
| } | ||
| const resource = mapModel.layers.on("changed", () => { | ||
| // Reset cache content so getSnapshot() fetches basemaps again. | ||
| baseLayers.current = undefined; | ||
| cb(); | ||
| }); | ||
| return () => resource.destroy(); | ||
| }, | ||
| [mapModel] | ||
| ); | ||
| const getSnapshot = useCallback(() => { | ||
| if (baseLayers.current) { | ||
| return baseLayers.current; | ||
| } | ||
| return (baseLayers.current = mapModel?.layers.getBaseLayers() ?? []); | ||
| }, [mapModel]); | ||
| return useSyncExternalStore(subscribe, getSnapshot); | ||
| } |
There was a problem hiding this comment.
This could hugely benefit from using the Reactivty API (see Comment).
If I am not mistake, caching current base layers and filtering + memoizing image labels could be packed into a single use of useReactiveSnapshot.
| const { selectedImageLabel, choosableImageLabel } = useMemo(() => { | ||
| const activeBaseLayer = map?.layers.getActiveBaseLayer(); | ||
| const selectedLayer = baseLayers.find((l) => l === activeBaseLayer); | ||
|
|
||
| const selectedImageLabel = setImageLabelObject(selectedLayer); | ||
|
|
||
| const choosableImageLabel = [] as ImageLabelSwitchObject[]; | ||
| baseLayers.forEach((layer) => { | ||
| if ( | ||
| excludeBasemapWithIdFilter && | ||
| !excludeBasemapWithIdFilter.includes(layer.id || "") | ||
| ) { | ||
| return; | ||
| } | ||
| choosableImageLabel.push(setImageLabelObject(layer)); | ||
| }); | ||
| if (allowSelectingEmptyBasemap) { | ||
| choosableImageLabel.push(setImageLabelObject(undefined)); | ||
| } | ||
|
|
||
| return { selectedImageLabel, choosableImageLabel }; | ||
| }, [ | ||
| map?.layers, | ||
| baseLayers, | ||
| setImageLabelObject, | ||
| allowSelectingEmptyBasemap, | ||
| excludeBasemapWithIdFilter | ||
| ]); |
| /** | ||
| * Optional filter to exclude basemaps from the switcher. | ||
| */ | ||
| excludeBasemapWithIdFilter?: string[]; |
There was a problem hiding this comment.
I propose using a filter function instead of a list of IDs.
This would allow greater flexibility.
|
@artmarks I will check how best to deal with the default thumbnails. |
See #361. Here is an example to show a image basemap switcher. I adjusted the
test-basemap-switcherapp to show the difference. Feedback and adjustments are welcome :)