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

Refactor: state of current location/review should be in Redux #406

Open
wbazant opened this issue Jun 18, 2024 · 1 comment · May be fixed by #419
Open

Refactor: state of current location/review should be in Redux #406

wbazant opened this issue Jun 18, 2024 · 1 comment · May be fixed by #419
Labels
refactoring Internal changes to code aesthetic or structure

Comments

@wbazant
Copy link
Contributor

wbazant commented Jun 18, 2024

Many FallingFruit activities are associated with a single location: showing details of it, editing the form, moving the marker around the map, etc. We have a good URL schema for them - '/locations/:locationId' identifies those pages - but the data about current location is not globally available. I'm coming to a conclusion that it creates weird places in the code / gaps / bugs.

  1. updateEntryLocation in src/redux/mapSlice.js, whose job it is to resolve URLs without the @ like /locations/1989068 to /locations/1989068/@55.82696200000001,-4.260660999999999,16z , is called from src/components/entry/EntryWrapper.js, whose job it is to display the location. A smell, plus I realised now it comes with a small bug: /locations/1989068/edit/ is not resolved with the same @.

  2. a "new location" marker is in the middle of the map and the map is made to move in the background, even on desktop where moving the marker would arguably be more intuitive

  3. there was no good way to add going back and forth between position and location screens for the PR to edit location position, see Add support for editing the position of an existing location #396 (comment)

  4. having the location open, and then clicking "edit", results in doing the API call again for no particular reason

  5. no place to store the state of location drawer, Location drawer reopens to middle position on back navigation #359

There's also a similar smell associated with the lack of the current review being globally available, but AFAIK this is only a thing in the context of a user editing their own review, and the map needing to know location ID of that review, which I worked around by making a look-up object in src/redux/miscSlice.js.

@ezwelty
Copy link
Collaborator

ezwelty commented Jun 20, 2024

Thank you for really digging into the implicit logic buried in the code. This really resonates with me. #359 has many flavors, all related to navigating back from the mobile view location to other pages: Street View (see #360), /imports/:id, etc. Just as editing a location calls the API again unnecessarily, so does editing a review.

Thinking back to #398 (comment), I wonder whether this would also facilitate accessing settings and returning to the currently selected location, although that may be best addressed by adopting the single-page-app style solution used on the current mobile app: putting the settings in a drawer that opens from the left over the rest of the app, which would restore the ability to switch basemaps, labels, etc without reloading the map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal changes to code aesthetic or structure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants