hackforla / 311-data

Empowering Neighborhood Associations to improve the analysis of their initiatives using 311 data
https://hackforla.github.io/311-data/
GNU General Public License v3.0
62 stars 63 forks source link

Add Mapbox Zoom Controls to Map Page #1793

Open ryanfchase opened 4 months ago

ryanfchase commented 4 months ago

Overview

We need to add zoom controls to our map page to improve accessibility and make it clear when user has an NC selected.

Overview

Resources (R)

  1. Design Hand-Off Materials 1.1 design ticket: https://github.com/hackforla/311-data/issues/1700 1.2 comment with screenshot of figma notes 1.3 Comment with screenshot of zoom controls (final) 1.4 Figma Link
  2. Mapbox Docs links 2.1 Docs example of setting zoom levels 2.2 Navigation Control Docs 2.3 Custom styles for mapbox controls
  3. UX Research's October '23 usability test 3.1 Slide indicating users required a zoom in/out feature

PM Review Section

Screenshot before proposed changes

![image](https://github.com/user-attachments/assets/e939d05b-c6c6-4c59-b683-a6d80028e398)

TODO: Screenshot after proposed changes

[insert screenshot here]

Resolved Dependency Info

Dependency (Resolved) - [x] Need to confirm with design team NC selected zoom behavior - don't disable zoom controls when NC is selected - prevent users from zooming too far out when NC is selected - [x] Need to establish zoom levels for NC selected - [x] Minimum zoom level - [x] Maximum zoom level

ryanfchase commented 2 months ago

This ticket is ready to be picked up

ryanfchase commented 2 months ago

Assigning this to @DrAcula27 based on conversation from earlier in the day.

@DrAcula27 please provide the following information:

Thank you!

DrAcula27 commented 2 months ago

ETA: 10 September Availability: weekdays after 2pm Pacific time

DrAcula27 commented 2 months ago

Updated ETA: 17 September

Availability: weekdays after 2pm Pacific time

DrAcula27 commented 1 month ago

Finally finished the large issue for the website team 🚀

DrAcula27 commented 1 month ago

@ryanfchase As mentioned in the meeting tonight, I'd like to schedule a quick meetup with someone to walk through the codebase, so I can more easily tackle this issue.

Availability:

DrAcula27 commented 1 month ago

ETA: October 11 with a check-in on the 2nd via either GitHub or Slack.

DrAcula27 commented 1 month ago

Help Wanted

@ryanfchase @traycn

Questions

if (zoomInButton && zoomOutButton) { zoomInButton.style.opacity = '0.5'; zoomInButton.style.pointerEvents = 'none'; zoomInButton.style.cursor = 'not-allowed'; zoomInButton.setAttribute('aria-disabled', 'true');

zoomOutButton.style.opacity = '0.5'; zoomOutButton.style.pointerEvents = 'none'; zoomOutButton.style.cursor = 'not-allowed'; zoomOutButton.setAttribute('aria-disabled', 'true'); }


What am I missing here?
> Some thoughts are, changing the `pointerEvents` style to `none` might be messing with things, Mapbox versioning issues, and styles overriding each other.
- Once I get this working, how can I use the `getZoom()` Mapbox method to allow the user to zoom between max allowed zoom and the zoom level the map goes to when a NC is selected?
ryanfchase commented 1 month ago

@DrAcula27 I think that setting pointer events to none was definitely preventing the cursor styles from being applied. I actually removed both these lines (from their respective blocks):

  zoomInButton.style.pointerEvents = 'none'; // remove this
  zoomInButton.style.cursor = 'not-allowed'; // remove this

I then simply zoomed in until I hit the max zoom level, and the pointer: not-allowed and aria-disabled="true" were applied automatically without needing custom CSS. I guess Mapbox simply wants control over these rules.


I've done some digging into programmatically setting min + max zooms. I cannot find any documentation on this, HOWEVER, you can simply call this.map.setMinZoom(z1) and this.map.setMaxZoom(z2).

You'll notice that when activating an NC, we frame the camera around that NC's bounding box. To get this exact zoom level, I think you can call this.map.getZoom(), and then plug that into setMinZoom(), perhaps with a few fractional zoom levels of wiggle room (up to you, we can review when we demo this).

Update: I worry there is some funkiness of grabbing this.map.getZoom right when we ask the map to zoom in on an NC. I think we would need to look into this.map.once('zoomend', () => ...) to ensure we grab the desired zoom level.

Another approach would be to prevent panning beyond the NC's bounding box. To do this, you can call boundingBox(geo), provided from geoUtils.js. I'm not sure off the top of my head where to obtain the geo feature, but I assume that you'll have access to the NC's geo object when you select it. I'm fairly certain geo, in this context, means a Feature, taking on the geojson shape as an object.

Resources

A single geojson Feature:

{
  "type": "Feature",
  "geometry": {
    "type": "Point",
    "coordinates": [125.6, 10.1]
  },
  "properties": {
    "name": "Dinagat Islands"
  }
}

Source on setting max bounds: https://stackoverflow.com/questions/61523571/how-to-fit-bounds-and-set-max-bounds-at-same-time-on-window-resize-in-mapbox

Where to find all NC geojson: data/ncGeojson.js

ryanfchase commented 1 month ago

I've just remembered that in the design ticket (https://github.com/hackforla/311-data/issues/1700#issuecomment-2311594526) we specified to use a max zoom of 18, and a min zoom of 9 when an NC is selected. PMs will want to check this behavior and determine if it is good enough. To avoid complexity mentioned above, feel free to move forward with these hard-coded zoom values before worrying about NC bounding boxes or pan areas.

DrAcula27 commented 1 month ago

Update to tooltip text:

DrAcula27 commented 1 month ago

Observation:

DrAcula27 commented 1 month ago

Observation:

DrAcula27 commented 1 month ago

ETA Update:

ryanfchase commented 3 weeks ago

In the PR, #1843, I suggested a different approach. I'll list in more detail how to implement my suggestion:

On map load, we'd like to wrap a zoom control with the Tooltip component. This lets us reuse styles that we've utilized for the Datepicker tooltip, etc.

Resources

MUI Docs

NPM

Stack Overflow

ryanfchase commented 3 weeks ago

Also, I have another approach which is slightly simpler. It is detailed below, but note that you cannot utilize any of the styles defined in our React components. We'll need to significantly modify the CSS to match the specs.

Resources

DrAcula27 commented 2 weeks ago

Updated ETA

DrAcula27 commented 2 weeks ago

@ryanfchase I've got the tooltip implemented, but with default styles. Let me know if I should make the PR as-is, or if I should work on implementing style changes.

Proposed styles according to Figma

image

Current default styles from MUI

image

ryanfchase commented 2 weeks ago

@DrAcula27 let's give design a little more time to review and officially hand off their work. In the meantime, please prepare all of your work in your PR and prepare for official review. I think you've done a lot of good work, I'd like to get these changes into the codebase before our Vite migration on Wednesday (10/30). The code for styling for this design will come in a separate ticket, most likely.

DrAcula27 commented 2 weeks ago

For updating the styles of the tooltip: https://mui.com/material-ui/react-tooltip/#customization

ryanfchase commented 1 week ago

I've provided my review. @DrAcula27 please move this ticket to In Review when you are ready for re-review. Thanks, and good work!