-
Notifications
You must be signed in to change notification settings - Fork 575
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
hackathon: build a cross-platform version of city guide #10552
Conversation
@@ -48,7 +48,7 @@ export const Search: React.FC = () => { | |||
const isAndroid = Platform.OS === "android" | |||
const navigation = useNavigation() | |||
|
|||
const shouldShowCityGuide = Platform.OS === "ios" && !isTablet() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far, excited about it! 💪
Left a few comments and another thing that might be nice is to remove any reminiscent native code ARTCityGuideView
if we have.
const mapRef = useRef<MapboxGL.MapView>(null) | ||
const cameraRef = useRef<MapboxGL.Camera>(null) | ||
|
||
const [pinsToRender, setPinsToRender] = useState<FilterData | undefined>(undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you don't need to declare undefined in the generic and either inside the parenthesis and selectedShow can follow the same idea in the way you're using it:
const [pinsToRender, setPinsToRender] = useState<FilterData>()
const [selectedShow, setSelectedShow] = useState<Show>()
if you mouse over pinsToRender
or selectedShow
later you'll see the undefined type there too
|
||
const [pinsToRender, setPinsToRender] = useState<FilterData | undefined>(undefined) | ||
const [selectedShow, setSelectedShow] = useState<Show | null>(null) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can declare something like:
const shows = extractNodes(data.city?.showsConnection)
useEffect(() => {
if (!shows.length) {
return
}
/** Get a GeoJSON-formatted list of show locations */
const showLocations = convertCityToGeoJSON(shows.map((show => (
...show,
icon: show.isFollowed ? "pin-saved" : "pin",
)))
clusterEngine.load(showLocations.features)
setPinsToRender({
featureCollection: showLocations,
filter: "all",
clusterEngine,
})
}, [data, setPinsToRender, shows]) // maybe there's more missing deps here, mind to add all of them
const showLocations = convertCityToGeoJSON(showData) | ||
|
||
/** Create a Supercluster instance to cluster show locations */ | ||
const clusterEngine = new Supercluster({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be instantiated outside of the component here in this file, isn't it?
lat | ||
lng | ||
} | ||
shows: showsConnection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recommend aliasing it here unless there's a conflict somewhere 😬
const MIN_ZOOM_LEVEL = 9 | ||
const MAX_ZOOM_LEVEL = 17.5 | ||
|
||
type Show = NonNullable< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the changes I suggested I think you won't need this type anymore
shows: showsConnection( | ||
includeStubShows: true | ||
status: RUNNING | ||
first: 2147483647 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂
@@ -48,7 +48,7 @@ export const Search: React.FC = () => { | |||
const isAndroid = Platform.OS === "android" | |||
const navigation = useNavigation() | |||
|
|||
const shouldShowCityGuide = Platform.OS === "ios" && !isTablet() | |||
const shouldShowCityGuide = !isTablet() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's already here, but can't we show the city guide on tablets too? 😄
I think the exp could be even better in tablet vs phones
going to close this one but will keep branch in case you want to revisit! |
This was my hackathon project to build a purely React Native version of City Guide so that collectors on Android can also use this wonderful feature.
At the moment this proof-of-concept can:
There are still more features to port over, so consider this a starting point where we can re-assemble the original functionalities.