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.

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

Conversation

ziggabyte
Copy link
Contributor

Description

This PR attempts to solve the issue we've been having with the canvasser map not re-drawing the lines around areas when you zoom in, and not loading new map tiles when you pan around the map.

Screenshots

Skarminspelning.2024-12-10.152211.mp4

Changes

  • Removes the returned function from the useEffect that attaches functionality to map actions
  • Moves a couple of lines of code for a slightly more organised file

Notes to reviewer

I have no idea why this works, but it works.
I re-built the map component from scratch, adding one bit at a time, and it failed when I added this useEffect, so i removed bit by bit in it, and it turns out if you remove the return value it solves the problem.

Related issues

none

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

Awesome work figuring this out. So methodical!

I'd love it if we could give a shot at reintroducing the unsubscribes (off()) but in a way that doesn't conflict with internal listeners. What do you think? Details below.

Comment on lines -195 to -199
return () => {
map.off('move');
map.off('moveend');
map.off('movestart');
};
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants