Merge pull request #3883 from terrestris/layer-switcher-visible-layer

fix: layerswitcher always shows next layer instead of current layer

src/LayerSwitcher/LayerSwitcher.spec.tsx

@@ -1,4 +1,5 @@
-import { render, screen,within } from '@testing-library/react';
+import { renderInMapContext } from '@terrestris/react-util/dist/Util/rtlTestUtils';
+import { screen, within } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import OlLayer from 'ol/layer/Layer';
 import OlLayerTile from 'ol/layer/Tile';
@@ -37,24 +38,24 @@ describe('<LayerSwitcher />', () => {
   });
 
   it('can be rendered', () => {
-    const { container } = render(<LayerSwitcher layers={layers} map={map} />);
+    const { container } = renderInMapContext(map, <LayerSwitcher layers={layers} />);
     expect(container).toBeVisible();
   });
 
   it('contains map element', () => {
-    const { container } = render(<LayerSwitcher layers={layers} map={map} />);
+    const { container } = renderInMapContext(map, <LayerSwitcher layers={layers} />);
     const mapElement = within(container).getByRole('menu');
     expect(mapElement).toBeVisible();
   });
 
   it('adds a custom className', () => {
-    render(<LayerSwitcher layers={layers} map={map} className="peter" />);
+    renderInMapContext(map, <LayerSwitcher layers={layers} className="peter" />);
     const firstChild = screen.getByRole('menu');
     expect(firstChild).toHaveClass('peter');
   });
 
   it('passes style prop', () => {
-    render(<LayerSwitcher layers={layers} map={map} style={{
+    renderInMapContext(map, <LayerSwitcher layers={layers} style={{
       backgroundColor: 'yellow',
       position: 'inherit'
     }} />);
@@ -68,16 +69,20 @@ describe('<LayerSwitcher />', () => {
   });
 
   it('sets all but one layer to invisible', () => {
-    render(<LayerSwitcher layers={layers} map={map} />);
+    renderInMapContext(map, <LayerSwitcher layers={layers} />);
     const layer0visibile = layers[0].getVisible();
     const layer1visibile = layers[1].getVisible();
     expect(layer0visibile && layer1visibile).toBe(false);
     expect(layer0visibile || layer1visibile).toBe(true);
   });
 
   it('switches the visible layer on click', async () => {
-    const { container } = render(<LayerSwitcher layers={layers} map={map} />);
-    const switcher = within(container).getByRole('button');
+    const { container } = renderInMapContext(map, <LayerSwitcher layers={layers} />);
+
+    // eslint-disable-next-line testing-library/no-container,testing-library/no-node-access
+    const layerSwitcher = container.querySelector('.react-geo-layer-switcher');
+
+    const switcher = within(layerSwitcher).getByRole('button');
 
     const layer0visible = layers[0].getVisible();
     const layer1visible = layers[1].getVisible();
@@ -90,7 +95,7 @@ describe('<LayerSwitcher />', () => {
 
   it('assumes the first provided layer as visible if the initial visibility of all layers is false', () => {
     layers.forEach(l => l.setVisible(false));
-    render(<LayerSwitcher layers={layers} map={map} />);
+    renderInMapContext(map, <LayerSwitcher layers={layers} />);
     expect(layers[0].getVisible()).toBeTruthy();
   });
 

src/LayerSwitcher/LayerSwitcher.tsx

@@ -1,5 +1,6 @@
 import './LayerSwitcher.less';
 
+import useMap from '@terrestris/react-util/dist/Hooks/useMap/useMap';
 import OlLayerBase from 'ol/layer/Base';
 import OlLayerGroup from 'ol/layer/Group';
 import OlLayerTile from 'ol/layer/Tile';
@@ -24,10 +25,6 @@ export interface OwnProps {
    * The layers to be available in the switcher.
    */
   layers: OlLayerBase[];
-  /**
-   * The main map the layers should be synced with.
-   */
-  map: OlMap;
   /**
    * The property that identifies the layer.
    */
@@ -47,11 +44,11 @@ export type LayerSwitcherProps = OwnProps & React.HTMLAttributes<HTMLDivElement>
 export const LayerSwitcher: React.FC<LayerSwitcherProps> = ({
   identifierProperty = 'name',
   labelProperty = 'name',
-  map,
   layers,
   className: classNameProp,
   ...passThroughProps
 }) => {
+  const map = useMap();
 
   const [switcherMap, setSwitcherMap] = useState<OlMap>();
 
@@ -67,19 +64,25 @@ export const LayerSwitcher: React.FC<LayerSwitcherProps> = ({
   const className = `${CSS_PREFIX}layer-switcher`;
 
   /**
-   * Sets the visiblity of the layers in the props.map and this._map.
+   * Sets the visibility of the layers in the map and the switcherMap.
    * Also sets the previewLayer in the state.
    */
   const updateLayerVisibility = useCallback(() => {
-    layers.forEach((l, i) => {
-      const visible = visibleLayerIndexRef.current === i;
-      l.setVisible(visible);
-      const clone = switcherMap?.getAllLayers()?.find(lc => lc.get(identifierProperty) === l.get(identifierProperty));
-      if (clone) {
-        clone.setVisible(visible);
-        if (visible) {
-          setPreviewLayer(clone);
-        }
+    layers.forEach((layer, i) => {
+      layer.setVisible(visibleLayerIndexRef.current === i);
+
+      const clone = switcherMap?.getAllLayers()
+        ?.find(lc => lc.get(identifierProperty) === layer.get(identifierProperty));
+
+      if (!clone) {
+        return;
+      }
+
+      if ((visibleLayerIndexRef.current + 1) % layers.length === i) {
+        clone.setVisible(true);
+        setPreviewLayer(clone);
+      } else {
+        clone.setVisible(false);
       }
     });
   }, [layers, identifierProperty, switcherMap]);
@@ -123,6 +126,10 @@ export const LayerSwitcher: React.FC<LayerSwitcherProps> = ({
   }, [switcherMap]);
 
   useEffect(() => {
+    if (!map) {
+      return;
+    }
+
     const mapClone =  new OlMap({
       view: map.getView(),
       controls: []
@@ -146,15 +153,9 @@ export const LayerSwitcher: React.FC<LayerSwitcherProps> = ({
   const onSwitcherClick = (evt: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
     evt.stopPropagation();
 
-    layers.forEach((layer, index: number) => {
-      if (layer.getVisible()) {
-        if (layers.length - 1 === index) {
-          visibleLayerIndexRef.current = 0;
-        } else {
-          visibleLayerIndexRef.current = index + 1;
-        }
-      }
-    });
+    const index = layers.findIndex(layer => layer.getVisible());
+
+    visibleLayerIndexRef.current = (index + 1) % layers.length;
 
     updateLayerVisibility();
   };