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

Allow rendering a list of locations without a previously opened map #384

Closed wbazant closed 4 months ago

wbazant commented 4 months ago

Addresses #347

Not quite finished, because it returns a 400 from the API: https://fallingfruit.org/api/0.3/locations/count?api_key=AKDJGHSD&muni=true&invasive=false&center=-0.9617529350012148,-90.95920155441307&limit=100&offset=0&photo=true "Cannot read properties of undefined (reading 'split')"

The openapi.yml promises bounds to be optional, so this is technically a bug to be fixed in the api, but a different solution would be to also initialize the bounds, maybe based on the default zoom.

wbazant commented 4 months ago

Okay, maybe the API will be easier to fix, although not for me today! A front-end only solution would be to also initialize the bounds, but apart from center of the view and zoom (available from the URL) they depend on screen size, and we get them from the map component.

Also I discovered some unused code and figured out why the list page needs a previously opened map - it registers an extra reducer that syncs the map view to the list view, but there's no way for the list to change the view.

wbazant commented 4 months ago

I want to address this together with #389, and start from moving the search component to Redux. Totally a yak shave, but the logic is as follows:

So, I think the success here - list that is independent from the map will look like listSlice.js and mapSlice.js having extra reducers for search and filter - needs that change, but it's big. I've made a boilerplate example to start it off at https://chatgpt.com/share/e8e97142-c403-41d1-8bdd-5157b35a1075 and the branch can wait for an ambitious evening.

ezwelty commented 4 months ago

@wbazant Restricting the list view to the map bounds is actually by design. For performance – they can share the same data (if number of locations less than the current request limit), although I don't believe this was ever implemented since their needs are slightly different (e.g. thumbnail photo); and importantly so that the list can act as a legend for what is on the map, which also was never really implemented but oft-requested (e.g. highlight location by clicking on a button in the list).

API locations/count endpoint requires bounds (query can become too expensive without). See the docs (https://petstore.swagger.io/?url=https://fallingfruit.org/api/0.3/openapi.yml#/Locations/get_locations_count). But API doesn't yet throw error if required params are missing.

Frankly I think the real issue here is that the map reloads every time user flips between map and the other mobile tabs (list and settings, primarily), which just feels wrong. The current mobile app is much better in this regard. If the map cannot be loaded in the background when the list is opened, then I guess the near-term hack is to calculate the bounds that the map would have if it were.

wbazant commented 4 months ago

I'll close this and I've made #390 for the unnecessary map reload.

wbazant commented 4 months ago

Superseded by https://github.com/falling-fruit/falling-fruit-web/pull/391. #391 doesn't address the issue - the google maps code doesn't actually get the bounds with display: none, I tried displaying it in the background and overlaying it with a rectangle but if felt like a crappy solution. I'll solve the issues here differently, but within the context of the map always being there, not sure yet - maybe just a redirect away from the URL we can't correctly display.