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

Remove state flag from useUpdateLeafletMapImperatively to avoid loop #1393

Merged
merged 1 commit into from Jun 11, 2021

Conversation

jwalgran
Copy link
Contributor

@jwalgran jwalgran commented Jun 10, 2021

Overview

When navigating from the facility search results list to the facility details view the final useEffect block in useUpdateLeafletMapImperatively was triggering a render loop. Removing the state flag used within that useEffect block eliminated the rendering loop without breaking any desired map pan and zoom functionality.

Connects #1297
Connects #1392

Demo

2021-06-10 13 38 51

Notes

I checked that the behaviors described in #799 are not affected.

  • If the user clicks on a facility marker, do not pan & zoom to the
    facility
  • If the user arrives at the map directly from a URL containing an OAR
    ID, set the map view on the facility location, zoomed to level 15
  • If the user clicks on a facility in the sidebar and the selected
    facility is outside of the bbox OR the zoom level is
    within the range in which the facilities grid layer is shown, pan to the
    facility and set the zoom level at 15. This ensures that the facility
    details marker will be visible even if the map was at grid-layer zoom
    levels before

While testing that this also fixes #1392 we saw that there was an error logged to the console and shown in the WDS overlay error viewer when navigating from the map page to a page without the map. It looks like this may be the result of some react-leaflet code trying to operate on the map related DOM elements that are no longer on the page.

Screen Shot 2021-06-10 at 1 49 18 PM

This error does not appear to affect functionality

Testing Instructions

  • Open the console, browse http://localhost:6543/, click the Facilities tab, select a facility, and verify that no error is logged.
  • Use the in-app back button to return to the facility list, select another facility, and verify that it also works.
  • Log in as c2@example.com
  • On the facility details page, click the "Claim this Facility" link and verify that the claim page loads (you may run into a non-blocking error when running in development. see note above. this error overlay can be dismissed using the X in the upper right.

Checklist

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

Copy link
Contributor

@TaiWilkin TaiWilkin left a comment

Choose a reason for hiding this comment

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

Great fix! I tested the zoom and pan functionality as well as the listed tests, and the map appears to be functioning as before.

@TaiWilkin TaiWilkin assigned jwalgran and unassigned TaiWilkin Jun 11, 2021
When navigating from the facility search results list to the facility details
view the final `useEffect` block in `useUpdateLeafletMapImperatively` was
triggering a render loop. Removing the state flag used within that `useEffect`
block eliminated the rendering loop without breaking any desired map pan and
zoom functionality.
@jwalgran
Copy link
Contributor Author

Thanks for the review.

@jwalgran jwalgran merged commit ab96efc into develop Jun 11, 2021
@jwalgran jwalgran deleted the bugfix/jcw/map-render-loop branch June 11, 2021 18:33
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.

Browser is unresponsive when clicking the Claim this Facility link on the facility details page
2 participants