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

fix: trigger cluster update when new cluster tiles are loaded (DHIS2-8498) #70

Merged
merged 3 commits into from
Mar 24, 2020

Conversation

turban
Copy link
Collaborator

@turban turban commented Mar 19, 2020

Fixes: https://jira.dhis2.org/browse/DHIS2-8498

When the app or user is quickly zooming the map, the cluster tiles are not yet ready to update. The result is that some clusters remains on the map. The issue is fixed by calling the onMoveEnd method every time a cluster tile is changed.

Screenshot showing the original issue:
Screenshot 2020-03-19 at 15 09 20

Screen recording showing that the map is properly updated:
Screen-Recording-2020-03-19-at-17 56 31

@jenniferarnesen
Copy link
Collaborator

Hi Bjørn, I set up the branch locally and I'm still able to see the bug. It doesn't always happen, just sometimes. If i click a second time on the "zoom to content" button, then the extra clusters are removed.

@turban
Copy link
Collaborator Author

turban commented Mar 20, 2020

I've pushed a new fix. The issue is that we rely on the underlying tile handling of Mapbox GL JS, and these tiles are not always updated when the "moveend" event is fired. The new fix checks if the visible tiles of the map aligns with the current zoom level. If not it will wait until this is the case.

Copy link

@martinkrulltott martinkrulltott left a comment

Choose a reason for hiding this comment

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

Tested locally, looks good when zooming in/out quickly and pressing the "Zoom to content" button. Approved with a few tiny comments 🚀

src/layers/ServerCluster.js Show resolved Hide resolved
src/layers/ServerCluster.js Outdated Show resolved Hide resolved
src/layers/ServerCluster.js Show resolved Hide resolved
@martinkrulltott
Copy link

Looks all good to me, maybe @jenniferarnesen wants to retest and verify before merging though. 👍

@turban turban merged commit 4e7ec77 into master Mar 24, 2020
@turban turban deleted the fix/DHIS2-8498 branch March 24, 2020 13:13
dhis2-bot added a commit that referenced this pull request Mar 24, 2020
## [1.0.15](v1.0.14...v1.0.15) (2020-03-24)

### Bug Fixes

* trigger cluster update when new cluster tiles are loaded (DHIS2-8498) ([#70](#70)) ([4e7ec77](4e7ec77))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants