feat: add home view section screen #10546

MounirDhahri · 2024-08-01T16:57:50Z

[Ready for view as of Monday 9th, September]

This PR resolves ONYX-1159

Description

This PR adds a new screen generic type that will render a list of artworks.

Screen.Recording.2024-09-09.at.12.07.53.mov

Screen.Recording.2024-09-09.at.15.44.06.mov

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

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.

src/app/Scenes/HomeSection/HomeSection.tsx

ArtsyOpenSource · 2024-08-01T17:01:21Z

	Warnings
⚠️	❌ An error occurred while validating your changelog, please make sure you provided a valid changelog.

Generated by 🚫 dangerJS against 76f9ea2

MounirDhahri · 2024-08-01T17:02:20Z

src/app/routes.ts

@@ -192,6 +192,7 @@ function getDomainMap(): Record<string, RouteMatcher[] | null> {
    addRoute("/galleries-for-you", "GalleriesForYou"),
    addRoute("/gene/:geneID", "Gene"),
    addRoute("/home-view", "HomeView"),
+    addRoute("/home-view/section/:sectionId", "HomeSectionScreen"),


glad to hear thoughts about the naming here

suggestion: sectionId > sectionID

src/app/Scenes/HomeSection/ArtworksScreenHomeSection.tsx

olerichter00 · 2024-08-02T13:49:34Z

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

+      artworksQuery,
+      { id: props.sectionId },
+      {
+        // This is necessary to avoid displaying cached because this component is used in more than one place and Relay returns data from another screen before the new data is loaded.


Shouldn't this return cached data per sectionID? 🤔

olerichter00 · 2024-08-02T13:50:58Z

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

+
+    // This won't happen because the query would fail thanks to the @principalField
+    // Adding it here to make TS happy
+    if (!data.homeView.section || !data.homeView.section.component?.title) {


If we require the title here, should we make it nonNullable in the schema instead?

I believe title was already non-nullable but this PR should alleviate the other null checking here.

dblandin

Looking good! 🔥

dblandin · 2024-08-02T13:29:12Z

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

+        // This is necessary to avoid displaying cached because this component is used in more than one place and Relay returns data from another screen before the new data is loaded.
+        fetchPolicy: "network-only",
+      }
+    )


question: Curious is there's a way to prevent relay cache data from leaking into the wrong components. I would think that maybe the arguments, like ID, would determine if a cached result is eligible or not.

Agree- maybe this is something to look into- tbh it wasn't an issue when I built this. I mainly copied the code here but probably it's not needed anymore https://github.com/artsy/eigen/blob/70571f97c97fb86cd8f0e997496107e33602f693/src/app/Components/WorksForYouArtworks.tsx#L126

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

src/app/Scenes/HomeView/Sections/ArtworksRailHomeViewSection.tsx

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

dblandin · 2024-08-02T16:38:16Z

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

+}
+
+export const ArtworksScreenHomeSection: React.FC<ArtworksScreenHomeSection> = ({ section }) => {
+  const defaultViewOption = GlobalStore.useAppState((state) => state.userPrefs.defaultViewOption)


question: Without having a control to toggle between the two options, should we explicitly pick a view style rather than referring to this default?

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

dblandin · 2024-08-02T16:41:07Z

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

+
+    // This won't happen because the query would fail thanks to the @principalField
+    // Adding it here to make TS happy
+    if (!data.homeView.section || !data.homeView.section.component?.title) {


I believe title was already non-nullable but this PR should alleviate the other null checking here.

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx

anandaroop

Looks good! Just one question below

anandaroop · 2024-09-09T21:43:58Z

src/app/routes.ts

@@ -192,6 +192,7 @@ function getDomainMap(): Record<string, RouteMatcher[] | null> {
    addRoute("/galleries-for-you", "GalleriesForYou"),
    addRoute("/gene/:geneID", "Gene"),
    addRoute("/home-view", "HomeView"),
+    addRoute("/home-view/sections/:sectionID", "HomeViewSectionScreen"),


thought (non-blocking): Skimming the route list it occurs to me that there isn't really a good way of telling which routes are dev-only (L194) or Eigen-only (L194–195). The vast majority of these routes are also valid public routes on web, but not all. I wonder if we'd consider some added structure to this list 🤔 (but not for this PR).

Yes, that's in the pipeline for next quarter with Sapphire. We are thinking of adjusting this file and making things easier here

anandaroop · 2024-09-10T02:16:52Z

src/app/Scenes/HomeView/Sections/ArtworksRailHomeViewSection.tsx

@@ -22,7 +22,7 @@ export const ArtworksRailHomeViewSection: React.FC<ArtworksRailHomeViewSectionPr
  const data = useFragment(fragment, section)
  const title = data.component?.title
  const artworks = extractNodes(data.artworksConnection)
-  const componentHref = data.component?.behaviors?.viewAll?.href
+  const componentHref = "home-view/sections/" + data.internalID


question: Shouldn't this prefer to navigate to the viewAll.href if it is provided, and only provide the generic full-screen experience here if viewAll.href is falsy?

Yes, I think we were agreeing on this behavior (response 2): artsy/metaphysics#5963 (comment)

Good point - I will the MP changes to reflect that the default behaviour is what we need here.

artsy/metaphysics#5988

I made some changes here and added a new helper for future use

MounirDhahri · 2024-09-10T12:50:00Z

Lol I messed up git history 😄 - sorry for the many commits

MounirDhahri · 2024-09-11T06:58:16Z

Going to merge this PR before I need to merge conflicts from 10 commits - I addressed all review comments above and will be glad to address any others that come up

MounirDhahri self-assigned this Aug 1, 2024

MounirDhahri commented Aug 1, 2024

View reviewed changes

src/app/Scenes/HomeSection/HomeSection.tsx Outdated Show resolved Hide resolved

MounirDhahri commented Aug 1, 2024

View reviewed changes

src/app/Scenes/HomeSection/HomeSection.tsx Outdated Show resolved Hide resolved

MounirDhahri commented Aug 1, 2024

View reviewed changes

src/app/Scenes/HomeSection/HomeSection.tsx Outdated Show resolved Hide resolved

MounirDhahri commented Aug 1, 2024

View reviewed changes

dblandin reviewed Aug 1, 2024

View reviewed changes

src/app/Scenes/HomeSection/ArtworksScreenHomeSection.tsx Outdated Show resolved Hide resolved

MounirDhahri added the Art Advisor label Aug 2, 2024

MounirDhahri marked this pull request as ready for review August 2, 2024 12:58

artsy-peril bot added the Jira Synced label Aug 2, 2024

olerichter00 reviewed Aug 2, 2024

View reviewed changes

dblandin reviewed Aug 2, 2024

View reviewed changes

MounirDhahri force-pushed the feat/add-home-view-section-screen branch from db88378 to 880220a Compare August 6, 2024 13:30

olerichter00 reviewed Aug 28, 2024

View reviewed changes

src/app/Scenes/HomeViewSectionScreen/Artworks/HomeViewSectionScreenArtworks.tsx Outdated Show resolved Hide resolved

MounirDhahri force-pushed the feat/add-home-view-section-screen branch 2 times, most recently from 1a0477e to 44c9d60 Compare September 9, 2024 10:07

MounirDhahri requested review from anandaroop and nickskalkin September 9, 2024 10:08

MounirDhahri force-pushed the feat/add-home-view-section-screen branch from 90727da to d26d9ae Compare September 9, 2024 12:56

MounirDhahri mentioned this pull request Sep 9, 2024

chore: update artwork rails href artsy/metaphysics#5984

Closed

anandaroop reviewed Sep 10, 2024

View reviewed changes

MounirDhahri added 8 commits September 10, 2024 14:42

chore: add screen container

bbe4838

feat: fetch section data

36bf170

chore: add artworks screen

3566e8d

chore: rename screens

afa9d43

chore: address review comments

0d1bde9

chore: use HOC

a3691ab

chore: use hooks

59e917b

chore: use new artwork screen sections

fbad15f

MounirDhahri added 12 commits September 10, 2024 14:42

chore: use new href format

b35296e

chore: send typename prop

d4477fc

chore: update component href logic

02f46c7

chore: add screen container

8b4ffed

feat: fetch section data

f370a80

chore: add artworks screen

ec48d19

chore: rename screens

c82d3ea

chore: address review comments

14f4b3b

chore: use HOC

75615b9

chore: use hooks

5fe73e2

chore: use new artwork screen sections

c7dc786

chore: use new href format

76f9ea2

MounirDhahri force-pushed the feat/add-home-view-section-screen branch from 3673518 to 76f9ea2 Compare September 10, 2024 12:48

MounirDhahri merged commit b0bea32 into main Sep 11, 2024
7 checks passed

MounirDhahri deleted the feat/add-home-view-section-screen branch September 11, 2024 06:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add home view section screen #10546

feat: add home view section screen #10546

MounirDhahri commented Aug 1, 2024 •

edited

Loading

ArtsyOpenSource commented Aug 1, 2024 •

edited

Loading

MounirDhahri Aug 1, 2024

dblandin Aug 1, 2024

MounirDhahri Aug 2, 2024

olerichter00 Aug 2, 2024

olerichter00 Aug 2, 2024

dblandin Aug 2, 2024

dblandin left a comment

dblandin Aug 2, 2024

MounirDhahri Aug 5, 2024

dblandin Aug 2, 2024

dblandin Aug 2, 2024

anandaroop left a comment

anandaroop Sep 9, 2024

MounirDhahri Sep 10, 2024

anandaroop Sep 10, 2024

nickskalkin Sep 10, 2024

MounirDhahri Sep 10, 2024

MounirDhahri Sep 10, 2024

MounirDhahri Sep 10, 2024

MounirDhahri commented Sep 10, 2024

MounirDhahri commented Sep 11, 2024

feat: add home view section screen #10546

feat: add home view section screen #10546

Conversation

MounirDhahri commented Aug 1, 2024 • 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

ArtsyOpenSource commented Aug 1, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblandin left a comment

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

Choose a reason for hiding this comment

anandaroop left a comment

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

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 commented Sep 10, 2024

MounirDhahri commented Sep 11, 2024

MounirDhahri commented Aug 1, 2024 •

edited

Loading

ArtsyOpenSource commented Aug 1, 2024 •

edited

Loading