Skip to content

Commit d1f61a0

Browse files
PessimistressPessimistress
authored
fix: Handle className prop change (#2483)
1 parent 0a59df5 commit d1f61a0

File tree

14 files changed

+207
-51
lines changed

14 files changed

+207
-51
lines changed

‎modules/react-mapbox/src/components/marker.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import type {MarkerEvent, MarkerDragEvent} from '../types/events';
99

1010
import {MapContext} from './map';
1111
import {arePointsEqual} from '../utils/deep-equal';
12+
import {compareClassNames} from '../utils/compare-class-names';
1213

1314
export type MarkerProps = MarkerOptions & {
1415
/** Longitude of the anchor location */
@@ -32,7 +33,6 @@ export const Marker = memo(
3233
forwardRef((props: MarkerProps, ref: React.Ref<MarkerInstance>) => {
3334
const {map, mapLib} = useContext(MapContext);
3435
const thisRef = useRef({props});
35-
thisRef.current.props = props;
3636

3737
const marker: MarkerInstance = useMemo(() => {
3838
let hasChildren = false;
@@ -102,6 +102,7 @@ export const Marker = memo(
102102

103103
useImperativeHandle(ref, () => marker, []);
104104

105+
const oldProps = thisRef.current.props;
105106
if (marker.getLngLat().lng !== longitude || marker.getLngLat().lat !== latitude) {
106107
marker.setLngLat([longitude, latitude]);
107108
}
@@ -123,7 +124,14 @@ export const Marker = memo(
123124
if (marker.getPopup() !== popup) {
124125
marker.setPopup(popup);
125126
}
127+
const classNameDiff = compareClassNames(oldProps.className, props.className);
128+
if (classNameDiff) {
129+
for (const c of classNameDiff) {
130+
marker.toggleClassName(c);
131+
}
132+
}
126133

134+
thisRef.current.props = props;
127135
return createPortal(props.children, marker.getElement());
128136
})
129137
);

‎modules/react-mapbox/src/components/popup.ts‎

Lines changed: 10 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import type {PopupEvent} from '../types/events';
99

1010
import {MapContext} from './map';
1111
import {deepEqual} from '../utils/deep-equal';
12+
import {compareClassNames} from '../utils/compare-class-names';
1213

1314
export type PopupProps = PopupOptions & {
1415
/** Longitude of the anchor location */
@@ -24,11 +25,6 @@ export type PopupProps = PopupOptions & {
2425
children?: React.ReactNode;
2526
};
2627

27-
// Adapted from https://github.com/mapbox/mapbox-gl-js/blob/v1.13.0/src/ui/popup.js
28-
function getClassList(className: string) {
29-
return new Set(className ? className.trim().split(/\s+/) : []);
30-
}
31-
3228
/* eslint-disable complexity,max-statements */
3329
export const Popup = memo(
3430
forwardRef((props: PopupProps, ref: React.Ref<PopupInstance>) => {
@@ -37,7 +33,6 @@ export const Popup = memo(
3733
return document.createElement('div');
3834
}, []);
3935
const thisRef = useRef({props});
40-
thisRef.current.props = props;
4136

4237
const popup: PopupInstance = useMemo(() => {
4338
const options = {...props};
@@ -75,32 +70,24 @@ export const Popup = memo(
7570
useImperativeHandle(ref, () => popup, []);
7671

7772
if (popup.isOpen()) {
73+
const oldProps = thisRef.current.props;
7874
if (popup.getLngLat().lng !== props.longitude || popup.getLngLat().lat !== props.latitude) {
7975
popup.setLngLat([props.longitude, props.latitude]);
8076
}
81-
if (props.offset && !deepEqual(popup.options.offset, props.offset)) {
77+
if (props.offset && !deepEqual(oldProps.offset, props.offset)) {
78+
popup.options.anchor = props.anchor;
8279
popup.setOffset(props.offset);
8380
}
84-
if (popup.options.anchor !== props.anchor || popup.options.maxWidth !== props.maxWidth) {
85-
popup.options.anchor = props.anchor;
81+
if (oldProps.anchor !== props.anchor || oldProps.maxWidth !== props.maxWidth) {
8682
popup.setMaxWidth(props.maxWidth);
8783
}
88-
if (popup.options.className !== props.className) {
89-
const prevClassList = getClassList(popup.options.className);
90-
const nextClassList = getClassList(props.className);
91-
92-
for (const c of prevClassList) {
93-
if (!nextClassList.has(c)) {
94-
popup.removeClassName(c);
95-
}
96-
}
97-
for (const c of nextClassList) {
98-
if (!prevClassList.has(c)) {
99-
popup.addClassName(c);
100-
}
84+
const classNameDiff = compareClassNames(oldProps.className, props.className);
85+
if (classNameDiff) {
86+
for (const c of classNameDiff) {
87+
popup.toggleClassName(c);
10188
}
102-
popup.options.className = props.className;
10389
}
90+
thisRef.current.props = props;
10491
}
10592

10693
return createPortal(props.children, container);

‎modules/react-mapbox/src/utils/compare-class-names.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/** Compare two classNames string and return the difference */
2+
export function compareClassNames(
3+
prevClassName: string | undefined,
4+
nextClassName: string | undefined
5+
): string[] | null {
6+
if (prevClassName === nextClassName) {
7+
return null;
8+
}
9+
10+
const prevClassList = getClassList(prevClassName);
11+
const nextClassList = getClassList(nextClassName);
12+
const diff: string[] = [];
13+
14+
for (const c of nextClassList) {
15+
if (!prevClassList.has(c)) {
16+
diff.push(c);
17+
}
18+
}
19+
for (const c of prevClassList) {
20+
if (!nextClassList.has(c)) {
21+
diff.push(c);
22+
}
23+
}
24+
return diff.length === 0 ? null : diff;
25+
}
26+
27+
function getClassList(className: string | undefined) {
28+
return new Set(className ? className.trim().split(/\s+/) : []);
29+
}

‎modules/react-mapbox/test/components/marker.spec.jsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ test('Marker', async t => {
4949
offset={[0, 1]}
5050
rotation={30}
5151
draggable
52+
className="classA"
5253
pitchAlignment="map"
5354
rotationAlignment="map"
5455
onDragStart={() => (callbackType = 'dragstart')}
@@ -64,6 +65,7 @@ test('Marker', async t => {
6465
t.not(rotation, marker.getRotation(), 'rotation is updated');
6566
t.not(pitchAlignment, marker.getPitchAlignment(), 'pitchAlignment is updated');
6667
t.not(rotationAlignment, marker.getRotationAlignment(), 'rotationAlignment is updated');
68+
t.ok(marker._element.classList.contains('classA'), 'className is updated');
6769

6870
marker.fire('dragstart');
6971
t.is(callbackType, 'dragstart', 'onDragStart called');

‎modules/react-mapbox/test/components/popup.spec.jsx‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,7 @@ test('Popup', async t => {
6767
</Map>
6868
);
6969
await sleep(1);
70-
71-
t.is(popup.options.className, 'classA', 'className is updated');
70+
t.ok(popup._container.classList.contains('classA'), 'className is updated');
7271

7372
root.unmount();
7473
t.end();

‎modules/react-mapbox/test/utils/compare-class-names.spec.js‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
import test from 'tape-promise/tape';
2+
import {compareClassNames} from '@vis.gl/react-mapbox/utils/compare-class-names';
3+
4+
test('compareClassNames', t => {
5+
const TEST_CASES = [
6+
{
7+
title: 'Empty class names',
8+
prevClassName: '',
9+
nextClassName: '',
10+
output: null
11+
},
12+
{
13+
title: 'Identical class names',
14+
prevClassName: 'marker active',
15+
nextClassName: 'active marker ',
16+
output: null
17+
},
18+
{
19+
title: 'Addition',
20+
prevClassName: undefined,
21+
nextClassName: 'marker',
22+
output: ['marker']
23+
},
24+
{
25+
title: 'Addition',
26+
prevClassName: 'marker',
27+
nextClassName: 'marker active',
28+
output: ['active']
29+
},
30+
{
31+
title: 'Removal',
32+
prevClassName: 'marker active',
33+
nextClassName: 'marker',
34+
output: ['active']
35+
},
36+
{
37+
title: 'Multiple addition & removal',
38+
prevClassName: 'marker active',
39+
nextClassName: 'marker hovered hidden',
40+
output: ['hovered', 'hidden', 'active']
41+
}
42+
];
43+
44+
for (const testCase of TEST_CASES) {
45+
t.deepEqual(
46+
compareClassNames(testCase.prevClassName, testCase.nextClassName),
47+
testCase.output,
48+
testCase.title
49+
);
50+
}
51+
t.end();
52+
});

‎modules/react-mapbox/test/utils/index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,4 @@ import './deep-equal.spec';
22
import './transform.spec';
33
import './style-utils.spec';
44
import './apply-react-style.spec';
5+
import './compare-class-names.spec';

‎modules/react-maplibre/src/components/marker.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import type {MarkerEvent, MarkerDragEvent} from '../types/events';
99

1010
import {MapContext} from './map';
1111
import {arePointsEqual} from '../utils/deep-equal';
12+
import {compareClassNames} from '../utils/compare-class-names';
1213

1314
export type MarkerProps = MarkerOptions & {
1415
/** Longitude of the anchor location */
@@ -32,7 +33,6 @@ export const Marker = memo(
3233
forwardRef((props: MarkerProps, ref: React.Ref<MarkerInstance>) => {
3334
const {map, mapLib} = useContext(MapContext);
3435
const thisRef = useRef({props});
35-
thisRef.current.props = props;
3636

3737
const marker: MarkerInstance = useMemo(() => {
3838
let hasChildren = false;
@@ -102,6 +102,7 @@ export const Marker = memo(
102102

103103
useImperativeHandle(ref, () => marker, []);
104104

105+
const oldProps = thisRef.current.props;
105106
if (marker.getLngLat().lng !== longitude || marker.getLngLat().lat !== latitude) {
106107
marker.setLngLat([longitude, latitude]);
107108
}
@@ -123,7 +124,14 @@ export const Marker = memo(
123124
if (marker.getPopup() !== popup) {
124125
marker.setPopup(popup);
125126
}
127+
const classNameDiff = compareClassNames(oldProps.className, props.className);
128+
if (classNameDiff) {
129+
for (const c of classNameDiff) {
130+
marker.toggleClassName(c);
131+
}
132+
}
126133

134+
thisRef.current.props = props;
127135
return createPortal(props.children, marker.getElement());
128136
})
129137
);

‎modules/react-maplibre/src/components/popup.ts‎

Lines changed: 9 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import type {PopupEvent} from '../types/events';
99

1010
import {MapContext} from './map';
1111
import {deepEqual} from '../utils/deep-equal';
12+
import {compareClassNames} from '../utils/compare-class-names';
1213

1314
export type PopupProps = PopupOptions & {
1415
/** Longitude of the anchor location */
@@ -24,11 +25,6 @@ export type PopupProps = PopupOptions & {
2425
children?: React.ReactNode;
2526
};
2627

27-
// Adapted from https://github.com/mapbox/mapbox-gl-js/blob/v1.13.0/src/ui/popup.js
28-
function getClassList(className: string) {
29-
return new Set(className ? className.trim().split(/\s+/) : []);
30-
}
31-
3228
/* eslint-disable complexity,max-statements */
3329
export const Popup = memo(
3430
forwardRef((props: PopupProps, ref: React.Ref<PopupInstance>) => {
@@ -37,7 +33,6 @@ export const Popup = memo(
3733
return document.createElement('div');
3834
}, []);
3935
const thisRef = useRef({props});
40-
thisRef.current.props = props;
4136

4237
const popup: PopupInstance = useMemo(() => {
4338
const options = {...props};
@@ -75,32 +70,24 @@ export const Popup = memo(
7570
useImperativeHandle(ref, () => popup, []);
7671

7772
if (popup.isOpen()) {
73+
const oldProps = thisRef.current.props;
7874
if (popup.getLngLat().lng !== props.longitude || popup.getLngLat().lat !== props.latitude) {
7975
popup.setLngLat([props.longitude, props.latitude]);
8076
}
81-
if (props.offset && !deepEqual(popup.options.offset, props.offset)) {
77+
if (props.offset && !deepEqual(oldProps.offset, props.offset)) {
8278
popup.setOffset(props.offset);
8379
}
84-
if (popup.options.anchor !== props.anchor || popup.options.maxWidth !== props.maxWidth) {
80+
if (oldProps.anchor !== props.anchor || oldProps.maxWidth !== props.maxWidth) {
8581
popup.options.anchor = props.anchor;
8682
popup.setMaxWidth(props.maxWidth);
8783
}
88-
if (popup.options.className !== props.className) {
89-
const prevClassList = getClassList(popup.options.className);
90-
const nextClassList = getClassList(props.className);
91-
92-
for (const c of prevClassList) {
93-
if (!nextClassList.has(c)) {
94-
popup.removeClassName(c);
95-
}
96-
}
97-
for (const c of nextClassList) {
98-
if (!prevClassList.has(c)) {
99-
popup.addClassName(c);
100-
}
84+
const classNameDiff = compareClassNames(oldProps.className, props.className);
85+
if (classNameDiff) {
86+
for (const c of classNameDiff) {
87+
popup.toggleClassName(c);
10188
}
102-
popup.options.className = props.className;
10389
}
90+
thisRef.current.props = props;
10491
}
10592

10693
return createPortal(props.children, container);

‎modules/react-maplibre/src/utils/compare-class-names.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/** Compare two classNames string and return the difference */
2+
export function compareClassNames(
3+
prevClassName: string | undefined,
4+
nextClassName: string | undefined
5+
): string[] | null {
6+
if (prevClassName === nextClassName) {
7+
return null;
8+
}
9+
10+
const prevClassList = getClassList(prevClassName);
11+
const nextClassList = getClassList(nextClassName);
12+
const diff: string[] = [];
13+
14+
for (const c of nextClassList) {
15+
if (!prevClassList.has(c)) {
16+
diff.push(c);
17+
}
18+
}
19+
for (const c of prevClassList) {
20+
if (!nextClassList.has(c)) {
21+
diff.push(c);
22+
}
23+
}
24+
return diff.length === 0 ? null : diff;
25+
}
26+
27+
function getClassList(className: string | undefined) {
28+
return new Set(className ? className.trim().split(/\s+/) : []);
29+
}

0 commit comments

Comments
 (0)