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

Add mapbox zoom 1793 v2 #1860

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

DrAcula27
Copy link
Member

@DrAcula27 DrAcula27 commented Nov 16, 2024

Fixes #1793

  • Up to date with main branch
  • Branch name follows guidelines
  • All PR Status checks are successful
  • Peer reviewed and approved

Any questions? See the getting started guide

@DrAcula27 DrAcula27 requested a review from ryanfchase November 16, 2024 19:48
@DrAcula27 DrAcula27 mentioned this pull request Nov 16, 2024
4 tasks
neighborhood council.
</strong>
<br />
To reset zoom features, please exit by clicking out of the
Copy link
Member

Choose a reason for hiding this comment

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

We needed to change this to say "please exit by clicking the 'x' on the selected NC" or something like that. More discussion incoming at upcoming general meeting

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the text to: To reset zoom features, please exit the boundary selection by clicking the 'X' on the selected Neighborhood Council within the 'Boundaries' filter of the 'Search & Filters' modal.

@DrAcula27 DrAcula27 requested a review from ryanfchase November 26, 2024 20:30
Copy link
Member

@ryanfchase ryanfchase left a comment

Choose a reason for hiding this comment

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

Mapbox fitBounds is behaving poorly. Steps to reproduce:

  1. select an NC (preferably small, e.g. "Greater Valley Glen")
  2. select another NC (preferably bigger, e.g. "Van Nuys NC")
  3. notice that we don't get the smooth transition between selected NCs

Explanation:

  • we need to unset the min zoom when flying from one NC to another. Mapbox glitches out if it tries flying somewhere, but it can't zoom out properly while flying due to min zoom being too small.

Steps to fix:

  • in components/Map/layers/BoundaryLayer.js, notice that there is a call to this.map.fitBounds() on line ~205
    • just above that line, set the map's min zoom to our default min zoom
  • optionally: move DEFAULT_MIN_ZOOM and DEFAULT_MAX_ZOOM into the constants folder (so we don't need to hard-code it in 2 places)

Example code starting at line ~203 in BoundaryLayer

    // zoom to the region
    this.map.setMinZoom(9) // here! 9 is DEFAULT_MIN_ZOOM
    this.map.fitBounds(boundingBox(geo), { padding: FIT_BOUNDS_PADDING }); // this was already here

@DrAcula27 DrAcula27 requested a review from ryanfchase December 13, 2024 22:40
@DrAcula27
Copy link
Member Author

@ryanfchase @traycn
I have fixed the bug @ryanfchase mentioned in the previous comment.

  • I moved the DEFAULT_MIN_ZOOM and DEFAULT_MAX_ZOOM constant variables from Map.jsx to the CONSTANTS.js file.
  • at line 205 in BoundaryLayer.js I added this.map.setMinZoom(DEFAULT_MIN_ZOOM)

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.

Add Mapbox Zoom Controls to Map Page
2 participants