Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make map tiles and polygons render correctly on pan/zoom #2423

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make map tiles and polygons render correctly on pan/zoom #2423

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 6 additions & 14 deletions src/features/canvassAssignments/components/CanvassAssignmentMap.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,20 @@ const CanvassAssignmentMap: FC<CanvassAssignmentMapProps> = ({
assignment.organization.id,
assignment.id
);
const [localStorageBounds, setLocalStorageBounds] = useLocalStorage<
[LatLngTuple, LatLngTuple] | null
>(`mapBounds-${assignment.id}`, null);

const [map, setMap] = useState<Map | null>(null);
const [zoomed, setZoomed] = useState(false);
const [selectedPlaceId, setSelectedPlaceId] = useState<string | null>(null);
const [isCreating, setIsCreating] = useState(false);
const [created, setCreated] = useState(false);

const [map, setMap] = useState<Map | null>(null);
const crosshairRef = useRef<HTMLDivElement | null>(null);
const reactFGref = useRef<FeatureGroupType | null>(null);

const [localStorageBounds, setLocalStorageBounds] = useLocalStorage<
[LatLngTuple, LatLngTuple] | null
>(`mapBounds-${assignment.id}`, null);
const selectedPlace = places.find((place) => place.id == selectedPlaceId);

const saveBounds = () => {
const bounds = map?.getBounds();
Expand All @@ -103,10 +105,6 @@ const CanvassAssignmentMap: FC<CanvassAssignmentMapProps> = ({
}
};

const [zoomed, setZoomed] = useState(false);

const selectedPlace = places.find((place) => place.id == selectedPlaceId);

const updateSelection = useCallback(() => {
let nearestPlace: string | null = null;
let nearestDistance = Infinity;
Expand Down Expand Up @@ -191,12 +189,6 @@ const CanvassAssignmentMap: FC<CanvassAssignmentMapProps> = ({
map.on('moveend', saveBounds);

map.on('zoomend', () => saveBounds);

return () => {
map.off('move');
map.off('moveend');
map.off('movestart');
};
Comment on lines -195 to -199
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value of a useEffect() can be a function, which will then be invoked whenever the useEffect() becomes stale, either because a component unmounts, or because the dependencies have changed and the useEffect() will get called again.

A common example is event listeners. If you add event listeners in a useEffect(), React may call that effect again, and if that happens it will add the same event listeners again, causing future interactions to be reacted to twice (likely a bug).

In those situations, you need to get a chance to remove the event listeners before React calls the effect again, and new listeners are added.

That's exactly what happens here. The on() and off() functions comprise an API which is the equivalent of addEventListener() and removeEventListener(). The off() is where the event listeners added above are removed, so I'm inclined not to want to remove this returned callback. However, I can see why this may be causing the bug, if the off() calls mean that all listeners for the specified events are removed (even internal ones), not just the specific callbacks added.

So maybe we can see if there is a way of invoking off() that specifically removes the functions added above, e.g. by naming those functions and passing them to off() as well?

PS. A separate problem is that the events being removed here are not the ones actually being added above.

}
}, [map, selectedPlaceId, places, panTo, updateSelection]);

Expand Down
Loading