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

Jump to bottom

Bugfix/update maxzoom cyleaflet #216

Merged
merged 19 commits into from
May 16, 2024
Merged

Bugfix/update maxzoom cyleaflet #216

merged 19 commits into from
May 16, 2024

Conversation

Farkites
Copy link
Contributor

@Farkites Farkites commented May 10, 2024 •
edited

About

This PR adds two bug fixes for the issues #208, #207, and #205 regarding the CyLeaflet AIO component.

Description of changes

#205:

  • A new dcc.Store has been added to the AIO component containing the data of the last leaflet extent state so that when the leaflet tiles contain a new maxZoom the extent isn't lost.
  • I have substituted the dictionary to map leaflet to cytoscape max zooms with a function that approximates the values in that dictionary and extends them to any maxZoom.
  • A callback that updates cytoscape's maxZoom when the children of leaflet change has been added.
  • The cytoscape maxZoom has been added as an input to the updateLeafBounds callback.
  • The demo usage_cy_leaflet_aio.py has been updated to include a leaflet maxZoom input.

#208:

  • The callback TransformElements allows duplicate outputs for the cytoscape elements.
  • A callback that updates AIO dcc.Store elements whenever cytoscape elements change has been added.
  • The demo usage_cy_leaflet_aio.py has been updated to include a number of elements for cytoscape input.

#207:

  • Recalculate latitude and longitude on the callback that updates elements dcc.Store.

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly.
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • At least one person has 💃'd the pull request.

Reference Issues

Closes #208
Closes #205
Closes #207

@Farkites Farkites requested a review from emilykl May 10, 2024 14:24
@emilykl
Copy link
Contributor

emilykl commented May 13, 2024

@Farkites I've noticed something that looks like a bug -- when the maxZoom is decreased, the map doesn't zoom out, which results in the map disappearing because now we're zoomed in greater than the max zoom.

Screen.Recording.2024-05-13.at.6.07.25.PM.mov

I know Leaflet is finicky and maybe there's no easy fix, but IMO the ideal would be that the map automatically zooms out to match the new max zoom.

@Farkites
Copy link
Contributor Author

@Farkites I've noticed something that looks like a bug -- when the maxZoom is decreased, the map doesn't zoom out, which results in the map disappearing because now we're zoomed in greater than the max zoom.

Screen.Recording.2024-05-13.at.6.07.25.PM.mov
I know Leaflet is finicky and maybe there's no easy fix, but IMO the ideal would be that the map automatically zooms out to match the new max zoom.

I found a way to fix this. On the event layoutstop I'm checking if the zoom is either over the max zoom or below the min zoom and calling cy.fit() if it's the case.

I haven't found a way to do this from the clientside callbacks since zoom is not a valid input/output.

@Farkites Farkites requested a review from emilykl May 14, 2024 16:06
emilykl
emilykl approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@emilykl emilykl 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 @Farkites , thanks for chasing down these loose ends. 🔥

Look into my comments before merging but I don't want to block you any further so I'll approve -- on the whole this looks great.

One more thing to check before merging -- can you confirm that the scenario described in #205 works properly now? I.e., switching out the leaflet children for a tile layer with a different maxZoom updates the behavior of the CyLeaflet as expected?

🦾 Thx!

@Farkites
Copy link
Contributor Author

One more thing to check before merging -- can you confirm that the scenario described in #205 works properly now? I.e., switching out the leaflet children for a tile layer with a different maxZoom updates the behavior of the CyLeaflet as expected?

Yes, it works as expected!

@Farkites Farkites merged commit 67fa01f into main May 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilykl emilykl emilykl approved these changes

@alexcjohnson alexcjohnson Awaiting requested review from alexcjohnson alexcjohnson is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Farkites @emilykl