Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Use selected marker for selected facility & set map view conditionally #749

Merged
merged 3 commits into from Aug 28, 2019

Conversation

kellyi
Copy link
Contributor

@kellyi kellyi commented Aug 26, 2019

Overview

When a facility has been selected -- via URL change, which can be
triggered by marker clicks -- set the facility marker in the vector tile
layer to use the selected marker.

When the user has arrived at the map by visiting a URL which contains an
OAR ID, set the map view to center on the facility when the facility
data returns. Otherwise, selecting a facility will not re-center the
viewport.

If the OAR ID encoded in a URL returns an error, switch off the setView
behavior to ensure that the next manually selected facility does not
incite a setView call.

Connects #739

Testing Instructions

  • serve this branch, switch on the vector_tile feature, then check the following behaviors:
  • clicking on a marker should send you to its details page and update the marker in the vector tile layer to indicate that it has been selected
  • clicking on a different marker should unselect the first marker, select the new marker, and change the facility details page
  • arriving at a URL with a valid OAR ID for details should center the map on that facility and display a marker with a selected icon
  • subsequently, clicking on other markers should not center the map view
  • arriving at a URL with an invalid OAR ID -- which causes an error -- should end up just showing the regular map. Clicking on any of the markers in this state should not re-center the map on the facility.
  • selecting a facility which is offscreen should center the map on that facility but not change the zoom level

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

@kellyi kellyi changed the title WIP Use selected marker for selected facility Use selected marker for selected facility & set map view conditionally Aug 26, 2019
When a facility has been selected -- via URL change, which can be
triggered by marker clicks -- set the facility marker in the vector tile
layer to use the selected marker.
@kellyi kellyi requested a review from jwalgran August 26, 2019 19:22
@kellyi kellyi marked this pull request as ready for review August 26, 2019 19:22
@kellyi kellyi removed the request for review from jwalgran August 26, 2019 19:40
@kellyi
Copy link
Contributor Author

kellyi commented Aug 26, 2019

Realized there's one more map interaction to re-create which is: center the map when selecting the facility if the facility is currently not visible in the bbox.

@jwalgran
Copy link
Contributor

Looking at this now.

Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

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

This is a nice, clear implementation.

icon: unselectedMarkerIcon,
});

tileLayer.setFeatureStyle(oarID, {
Copy link
Contributor

Choose a reason for hiding this comment

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

setFeatureStyle is such a nice API. Thanks, Leaflet.VectorGrid.

noop();
} else if (fetching && !isFetchingFacility) {
setIsFetchingFacility(true);
} else if (!fetching && isFetchingFacility && data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Names are hard. If you read this method without a lot of context, it is a little confusing to be !fetching and isFetchingFacility at the same time. Is there a way we can rename fetching to more descriptive? Looks like fetching is the value coming from Redux and isFetchingFacility is our component local flag. This isn't the first time we have had a pattern like this and it seems like a thing that is going to pop up the more we use local state hooks. I don't have any good ideas right now. This may be something to ask the larger team or #react about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me. Generally I've been thinking that it might be worthwhile for us to start writing more custom hooks so that we can extract the wonky logic into something that has a more reasonable name for its interface. In this case it probably makes sense just to rename either fetching or isFetchingFaciliity to distinguish them, so I'll think about how to do that and make a change before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed isFetchingFacility to appIsGettingFacilityData, as well as the related setter.

@jwalgran
Copy link
Contributor

I found an edge-case bug after approving this.

  • Select a facility
  • Refresh. The map correctly zooms in
  • Hard refresh (Cmd+Shift+R in Mac Firefox). The map incorrectly shows the initial zoom, but the facility is selected.

@jwalgran jwalgran assigned kellyi and unassigned jwalgran Aug 27, 2019
@kellyi
Copy link
Contributor Author

kellyi commented Aug 27, 2019

Hard refresh (Cmd+Shift+R in Mac Firefox). The map incorrectly shows the initial zoom, but the facility is selected.

Interesting. I'll look into this.

@kellyi
Copy link
Contributor Author

kellyi commented Aug 28, 2019

I was able to re-create the hard-refresh zoom issue in FF, so I'll see if I can mitigate it.

It does not appear to affect Chrome.

Kelly Innes added 2 commits August 28, 2019 13:26
When the user has arrived at the map by visiting a URL which contains an
OAR ID, set the map view to center on the facility when the facility
data returns. Otherwise, selecting a facility will not re-center the
viewport.

If the OAR ID encoded in a URL returns an error, switch off the setView
behavior to ensure that the next manually selected facility does not
incite a setView call.

If a facility which is off-screen is selected (as may happen when
selecting a facility from the list sidebar), center the map view on the
facility when its data returns.
@kellyi
Copy link
Contributor Author

kellyi commented Aug 28, 2019

I fixed the Firefox/hard-refresh quirk by using a delay of 0 around setting the map view when user arrives directly at the details page via a URL. Everything else still works as expected.

@kellyi kellyi merged commit af0667a into develop Aug 28, 2019
@kellyi kellyi deleted the ki/handle-marker-clicks branch August 29, 2019 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants