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 routes code, save form when back from add location #658

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Conversation

wbazant
Copy link
Collaborator

@wbazant wbazant commented Jan 2, 2025

Closes #604
Closes #601
Closes #652

The code would now support a "back button saves edits" for edit location / add review / edit review, and originally I saw myself adding those, but then realised they're not really useful features.

@ezwelty
Copy link
Collaborator

ezwelty commented Jan 26, 2025

@wbazant No issues found with #601 and #604. Regarding #652:

  • Sentence case please (it is de rigeur in the existing translations). Also these text snippets (except for "Editing Review") already exist in the mobile app locale files:
  "menu.add_location": "Add location",
  "menu.add_review": "Add review",
  "menu.edit_location": "Edit location",

https://github.com/falling-fruit/falling-fruit-mobile/blob/f29322747bc0bbfc026563c68b8bf26f5c976070/www/locales/en.json#L38

  • I'm not super fond of /locations/:locationId/edit/details because it diverges from the existing website. Can we use /locations/:locationId/edit for the form (form + map on desktop) and /locations/:locationId/edit/position for the position (mobile only), or does this compromise the new website route-logic somehow?

@wbazant wbazant merged commit 048295a into main Jan 27, 2025
2 checks passed
@wbazant wbazant deleted the issue-604 branch January 27, 2025 16:45
@wbazant
Copy link
Collaborator Author

wbazant commented Jan 27, 2025

Thanks! I agree with you /locations/:locationId/edit is a better path - I'm not even sure why I standardised them this way instead of the other way, maybe having .../details and .../position was a bit simpler for writing the routes, but that's not a user benefit.

I'll merge this and I've made an issue #672 to use the verbiage from the mobile upp for the headers - it needs a bit of unrelated work, so I'll do it on a new branch as part of the translation epic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants