feat: Replace Touchable with Routerlink for prefetching on artwork and artist rails #11383

olerichter00 · 2025-01-13T14:32:09Z

This PR resolves ONYX-1461

Description

This PR replaces Touchable with Routerlink for prefetching on artwork and artist rails, as well as some additional rails on the home screen. It also makes some changes to the RouterLink component and to routes.ts. All updates are explained in the comments.

PR Checklist

I have tested my changes on iOS and Android.
I hid my changes behind a feature flag, or they don't need one.
I have included screenshots or videos, or I have not changed the UI.
I have added tests, or my changes don't require any.
I added an app state migration, or my changes do not require one.
I have documented any follow-up work that this PR will require, or it does not require any.
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

Cross-platform user-facing changes

Replace Touchable with Routerlink for prefetching on artwork and artist rails, as well as some additional rails on the home screen - ole

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

olerichter00 · 2025-01-14T13:59:50Z

src/app/Navigation/routes.tsx

+  queries?: GraphQLTaggedNode[]
+  queryVariables?: object[]


Renaming Queries to queries and adding queryVariables to enable defining default variables for each query (e.g., for sorting or default filters). This is important to prefetch queries with the exact same variables as the screen (so far this is only needed for the Artist screen).

I will have a look to see if we can specify the default variables within the query instead. This should work in most cases and would be cleaner.

suggestion:
How about we combine the queries and queryVariables at a single variable to avoid having to match two arrays.

queries: [{name: Screen1Query}, {name: Screen2Query, variables: Screen2Variables }]

instead of

queries: [Screen1Query, Screen2Query] querVariables: [null, Screen2Variables]

This is a very good idea. The reason I went with the "simpler" solution is that adding querVariables is quite an edge case (only one case so far), and I wanted to keep the route definition as concise as possible. I know it's not very clean, but in almost all cases, it will look like

queries: [Screen1Query, Screen2Query]

instead of (query instead of name because we're defining the query itself).

queries: [{ query: Screen1Query }, { query: Screen2Query }]

Please let me know what you think. I would also be okay with your solution and my urge to keep it concise might not be the best idea.

I've added comments to the new route attributes in d53dd73

Sounds good to me - let's keep an eye on the API here in case more stuff gets added

Yeah, if we need to add more defaultVariables in the future or more values, the object should be updated. 👍

src/app/Components/ShowCard.tsx

olerichter00 · 2025-01-14T16:07:10Z

src/app/Scenes/ViewingRoom/Components/ViewingRoomArtworkRail.tsx

@@ -36,6 +36,7 @@ export const ViewingRoomArtworkRail: React.FC<ViewingRoomArtworkRailProps> = ({
      </Flex>
      <ArtworkRail
        artworks={artworks}
+        itemHref={(artwork) => `/viewing-room/${viewingRoom.slug}/${artwork.slug}`}


The viewing room artwork rail links to the viewing room artwork screen instead of the artwork screen and uses the itemHref prop to customize the linking.

MounirDhahri

Great work! I left a small comment about routes.ts for a code preference. It's not blocking though

src/app/utils/loggers.ts

MounirDhahri · 2025-01-14T16:41:24Z

src/app/Navigation/routes.tsx

+  queries?: GraphQLTaggedNode[]
+  queryVariables?: object[]


MounirDhahri · 2025-01-14T16:46:16Z

src/app/Navigation/routes.tsx

+  queries?: GraphQLTaggedNode[]
+  queryVariables?: object[]


suggestion:
How about we combine the queries and queryVariables at a single variable to avoid having to match two arrays.

queries: [{name: Screen1Query}, {name: Screen2Query, variables: Screen2Variables }]

instead of

queries: [Screen1Query, Screen2Query] querVariables: [null, Screen2Variables]

ArtsyOpenSource · 2025-01-15T09:09:16Z

This PR contains the following changes:

Cross-platform user-facing changes (Replace Touchable with Routerlink for prefetching on artwork and artist rails, as well as some additional rails on the home screen - ole - olerichter00)

Generated by 🚫 dangerJS against d53dd73

damassi · 2025-01-15T22:12:57Z

src/app/Navigation/routes.tsx

@@ -336,7 +347,8 @@ export const artsyDotNetRoutes = defineRoutes([
        headerShown: false,
      },
    },
-    Queries: [ArtistScreenQuery],
+    queries: [ArtistScreenQuery],
+    queryVariables: [defaultArtistVariables],


olerichter00 self-assigned this Jan 13, 2025

olerichter00 marked this pull request as draft January 13, 2025 14:32

artsy-peril bot added the Jira Synced label Jan 13, 2025

olerichter00 commented Jan 14, 2025

View reviewed changes

src/app/Components/ShowCard.tsx Show resolved Hide resolved

olerichter00 added 6 commits January 14, 2025 16:43

feat: Prefetch home screen links

cf76215

prefetch artist query

a91e54d

prefetch artist & artwork query

de6dcf8

recently sold

a9ac262

tests

df74bf8

re

494f4cb

olerichter00 changed the title ~~feat: Prefetch home screen links~~ feat: Replace Touchable with Routerlink for prefetching on artwork and artist rails Jan 14, 2025

olerichter00 requested a review from MounirDhahri January 14, 2025 16:05

olerichter00 commented Jan 14, 2025

View reviewed changes

MounirDhahri previously approved these changes Jan 14, 2025

View reviewed changes

olerichter00 marked this pull request as ready for review January 15, 2025 09:06

revert logger

9e24362

olerichter00 dismissed MounirDhahri’s stale review via 9e24362 January 15, 2025 09:07

olerichter00 force-pushed the olerichter00/ONYX-1461/prefetch-home-screen-links branch from 258f885 to 9e24362 Compare January 15, 2025 09:07

comments

d53dd73

olerichter00 merged commit aa6c47d into main Jan 15, 2025
7 checks passed

olerichter00 deleted the olerichter00/ONYX-1461/prefetch-home-screen-links branch January 15, 2025 12:41

damassi reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Replace Touchable with Routerlink for prefetching on artwork and artist rails #11383

feat: Replace Touchable with Routerlink for prefetching on artwork and artist rails #11383

olerichter00 commented Jan 13, 2025 •

edited

Loading

olerichter00 Jan 14, 2025 •

edited

Loading

olerichter00 Jan 14, 2025

MounirDhahri Jan 14, 2025

MounirDhahri Jan 14, 2025

olerichter00 Jan 15, 2025 •

edited

Loading

olerichter00 Jan 15, 2025 •

edited

Loading

MounirDhahri Jan 15, 2025

olerichter00 Jan 15, 2025

olerichter00 Jan 14, 2025

MounirDhahri left a comment

MounirDhahri Jan 14, 2025

MounirDhahri Jan 14, 2025

ArtsyOpenSource commented Jan 15, 2025 •

edited

Loading

damassi Jan 15, 2025

feat: Replace Touchable with Routerlink for prefetching on artwork and artist rails #11383

feat: Replace Touchable with Routerlink for prefetching on artwork and artist rails #11383

Conversation

olerichter00 commented Jan 13, 2025 • edited Loading

Description

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

olerichter00 Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olerichter00 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

olerichter00 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounirDhahri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtsyOpenSource commented Jan 15, 2025 • edited Loading

This PR contains the following changes:

Choose a reason for hiding this comment

olerichter00 commented Jan 13, 2025 •

edited

Loading

olerichter00 Jan 14, 2025 •

edited

Loading

olerichter00 Jan 15, 2025 •

edited

Loading

olerichter00 Jan 15, 2025 •

edited

Loading

ArtsyOpenSource commented Jan 15, 2025 •

edited

Loading