-
Notifications
You must be signed in to change notification settings - Fork 11
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
Improve map performance #1069
base: master
Are you sure you want to change the base?
Improve map performance #1069
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build succeeded and deployed at https://aqualink-app-1069.surge.sh |
@K-Markopoulos could you rebase on the last version of the reef-check frontend so that we can deploy it to programize for testing? |
@ericboucher Sure but what's the plan for testing? This PR doesn't have any overlapping changes with the reef-check frontend PRs. The generated link: https://aqualink-app-1069.surge.sh/ shows the reef-check sites on the map as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@K-Markopoulos you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PullRequest Breakdown
Reviewable lines of change
+ 178
- 98
66% TSX
34% TypeScript
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work here. I don’t have any security concerns with these changes.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@K-Markopoulos I like the clustering icons. I like when we clearly show how many sites that experience what heat stress. I think the white center clarify this the most. If we would color the center, then we should display the most abundant heat stress level for the specific area. I think we should maybe separate the center and the outside circle a little bit more if we’d choose this choice. See example below. Which do you think would be best? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.
The goal is to improve initial load of the map and provide smooth scrolling and zooming. The main cause is that we are rendering (and re-rendering many times) 5.000+ markers at once.
Changes
siteOnMap
selector withisSelected
selector which caused all markers to re-render when selected site was changed.