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

Put state of current location/review in Redux #410

Merged
merged 10 commits into from
Jul 3, 2024

Conversation

wbazant
Copy link
Contributor

@wbazant wbazant commented Jun 20, 2024

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!

(co-authored with Claude Sonnet 3.5 via aider-chat
by a sequence of fairly mechanistic prompts and tested by reading results
and then clicking around tabs, so should be okay)
@wbazant
Copy link
Contributor Author

wbazant commented Jun 20, 2024

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 .

Done by clearing location in Redux when it goes out of context
(visits to /map and /list but not always)

x
@wbazant
Copy link
Contributor Author

wbazant commented Jun 26, 2024

Now also closes #407

@wbazant
Copy link
Contributor Author

wbazant commented Jun 27, 2024

Now also closes #412

@ezwelty
Copy link
Collaborator

ezwelty commented Jul 3, 2024

@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 #412 (comment): I think the marker should appear immediately, but the location should only display once the latest pending is fulfilled.

@wbazant
Copy link
Contributor Author

wbazant commented Jul 3, 2024

@ezwelty I have these commits on top: wbazant#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
Copy link
Collaborator

ezwelty commented Jul 3, 2024

@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
Copy link
Contributor Author

wbazant commented Jul 3, 2024

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

@ezwelty ezwelty merged commit 71691e8 into falling-fruit:main Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants