Skip to content
This repository has been archived by the owner on Apr 28, 2018. It is now read-only.

After moving with the app loaded, places are sorted by absolute location rather than travel times #627

Open
mcomella opened this issue Mar 9, 2017 · 3 comments

Comments

@mcomella
Copy link
Contributor

mcomella commented Mar 9, 2017

I think this is a mistake from a UX perspective.

Maybe be hard b/c of async nature of travel times.

@mcomella mcomella added the bug label Mar 9, 2017
@mcomella mcomella added this to the Sprint #3 – map view milestone Mar 9, 2017
@mcomella mcomella self-assigned this Mar 10, 2017
@mcomella
Copy link
Contributor Author

Current state:

  • Apple calls didUpdateLocation multiple times on startup (giving finer locations each time, I believe), forcing the places list to be sorted.
    • This means the places list is pretty much always sorted by distance (first call we fetch places, next calls we re-sort)
    • To fix this: we can refuse to sort places if we've already done so within some time frame and reset the value when we fetch new places.
      • If we have a large place list, and we get rate limited, the sort order could change each time this is called. Since it should only get called when we walk, this is probably fine.
      • This is a complex change so close to user testing
  • Pre-existing bug: In the current sortPlaces implementation, we update displayedPlaces but don't update placeKeyMap, which is [place.id: displayedPlacesIndex]. This means the current app could have unexpected behavior as the indices don't align.
    • This is fixed every time we fetch the places again, which is when the app goes background -> foreground

I think there are three options:

  1. Do nothing
  2. Fix the bug and do nothing else
  3. Attempt my suggested fix

I have the code written for 3, but I'm concerned about unexpected edge cases and it'll take some more time to test (which takes a while, given I have to walk around).

After speaking with Maria, she mentioned I should focus on #558, then focus on this bug. As such, I'll land the bug fix and then maybe get to testing this.

mcomella added a commit to mcomella/prox that referenced this issue Mar 11, 2017
…erver.

This ensures these linked properties update together.

This also fixes a bug where we didn't update the mapping when we updated the
displayedPlaces while re-sorted the displayedPlaces list.
mcomella added a commit to mcomella/prox that referenced this issue Mar 11, 2017
mcomella added a commit to mcomella/prox that referenced this issue Mar 11, 2017
mcomella added a commit to mcomella/prox that referenced this issue Mar 11, 2017
mcomella added a commit to mcomella/prox that referenced this issue Mar 11, 2017
We previously sorted by distance from location, for an unknown reason.

The old implementation had a bug: it never updated the
placeKeyToDisplayedPlacesIndexMap but updated the displayedPlaces list so the
existing behavior is unknown.

NB: I didn't test this yet.
@mcomella
Copy link
Contributor Author

mcomella commented Mar 11, 2017

I landed the intermediate fix (untested IRL) and am working on the big fix in https://github.com/mcomella/prox/tree/i627-sort (note: I think it only needs to be tested IRL).

@mcomella
Copy link
Contributor Author

I'm not going to land this now – alternatively, we should consider deliberately designing this experience rather than landing hacky fixes.

@mcomella mcomella removed their assignment Jul 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

1 participant