search

falling-fruit / falling-fruit-web

Mobile-friendly website for Falling Fruit
https://beta.fallingfruit.org
GNU General Public License v3.0
31 stars 12 forks source link

Put state of current location/review in Redux #410

Closed wbazant closed 3 months ago

wbazant commented 3 months ago

The reducers are just in their simplest state - and will probably grow if we put more stuff there, like intermediate states of form or drawer state - and there aren't really any huge benefits from the PR by itself but it should help later. From the original points:

  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 @.

I checked we still do a redirect from a bare URL to the one with coordinates

  1. 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

Opened new issue, #409

  1. there was no good way to add going back and forth between position and location screens for the PR to edit location position Should be easier now!

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

  3. no place to store the state of location drawer Should be easier to do now!

wbazant commented 3 months ago

This PR is ready to go as is, but I started doing more work on top of it, as a warm-up for 3. above.

The goal was #407 - in "new position" state, going to settings and back should bring back the marker, which was the most trivial feature that uses saved location data I could think of. Then I found the tabs code, and made the change that closes #411 .

wbazant commented 3 months ago

Now also closes #407

wbazant commented 3 months ago

Now also closes #412

ezwelty commented 3 months ago

@wbazant This feels like a major step forward! Do you want to merge now or keep building on this branch (e.g. for #359)?

After trying out the suggested fix to #412, I agree with my https://github.com/falling-fruit/falling-fruit-web/issues/412#issuecomment-2205871068: I think the marker should appear immediately, but the location should only display once the latest pending is fulfilled.

wbazant commented 3 months ago

@ezwelty I have these commits on top: https://github.com/wbazant/falling-fruit-web/pull/1 .

I tried to keep the branch to be good to merge at any point, so we could merge it now and I'll open a new draft PR for editing the location with the intermediate state in Redux?

ezwelty commented 3 months ago

@wbazant Would it be convenient for you to adjust the #412 behavior in this branch before we merge? If not, and you would rather tackle that in a later PR, I could merge now. But I'm in no hurry to merge; whatever is easiest for you.

wbazant commented 3 months ago

Sure, I've made the relevant lines like on the other branch.