artsy
feat(city guide): enable expanded list of cities
Description
Co-authored-by: @MounirDhahri
To accompany the enablement of cross-platform City Guide (#12175) here we also expand the list of cities from the original six that were supported at launch.
Which cities to include? Luckily we already have a concept of "featured cities" which we make use of in various places e.g. on https://www.artsy.net/shows
So here we create an expanded list of cities based on that existing concept.
For QA purposes, this is behind a feature flag: Enable expanded city list in City Guide
| iOS | Android |
|---|---|
PR Checklist
- [x] I have tested my changes on the following platforms:
- [x] Android.
- [x] iOS.
- [x] I hid my changes behind a feature flag, or they don't need one.
- [x] I have included screenshots or videos at least on Android, or I have not changed the UI.
- [ ] I have added tests, or my changes don't require any.
- [x] I added an app state migration, or my changes do not require one.
- [x] I have documented any follow-up work that this PR will require, or it does not require any.
- [x] I have added a changelog entry below, or my changes do not require one.
To the reviewers 👀
- [ ] I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
- Expanded list of City Guide cities behind feature flag
Need help with something? Have a look at our docs, or get in touch with us.
| Warnings | |
|---|---|
| :warning: |
Changes were made to package.json, but not to yarn.lock. |
Generated by :no_entry_sign: dangerJS against 26005084905bfaa5bd54cff80306145a944f3091
![artsy-peril[bot] avatar](https://avatars.githubusercontent.com/u/546231?v=4 "Published by a pub.dev verified publisher"){kind=small}
FYI, I looked further into the scripts we currently have but are not using to understand if they are worth keeping. I believe we don't need them anymore. The goal, I assume, was to preheat the relay cache so more cities are available for quicker switching as well as sending less payload to MP (using query id instead of the entire query payload). Although it's a nice quality of life improvement, I think it could mean that one might get cached values and we would need to maintain a script that is very hard to know about its existence (which is the case now). Which is why I think it's fair to remove it
One final thought I have before opening this up for QA — now that we've got a bigger list of cities in the picker UI, it's no longer easy to digest in one glance.
Therefore I think the ordering of the list is important. Currently it's essentially in random order:
(Not really random, it follows this list in Gravity, but I do not know why that list is ordered the way it is, and neither will any user.)
Since we have full control in Eigen thanks to that static file that lives in this repo, we can modify the ordering to make it more sensible.
There's more than one way to do this. I propose keeping the "original 6" cities at the top of the list, followed by the remainder in alphabetical order:
Not perfect, but an improvement over the current list I think, a compromise between keeping the original 6 major cities where they always were vs making the other cities easy to find by skimming.
@MounirDhahri if you agree, I will push that change up to this branch.
@anandaroop I think that's a valid concern and I like the suggested solution.
My only worry is that if you didn't see your city at first glance from the top 6 in the List (London for example), you might not find it after you scroll down to the letter L. I suggest maybe to also include top cities along with the full list of cities below and make the separation clear. Something like this
Another alternative would be a search bar but I like your suggestion more
I was going for the lazy approach that merely tweaked the data file, but yeah I'm open to any of that! Either now, or as a a follow-up. We could get design input as well. Maybe a hands-on QA session could surface ideas about the best way forward.
| Warnings | |
|---|---|
| :warning: |
Changes were made to package.json, but not to yarn.lock. |
Generated by :no_entry_sign: dangerJS against 26005084905bfaa5bd54cff80306145a944f3091
Updated to prefer strict alphabetical ordering of city list, per today's discussion: