feat(ONYX-1479): add quick links section #6354

dariakoko · 2025-01-15T09:44:59Z

Addresses ONYX-1479

Adds QuickLinks section to the list of HomeView sections

Co-authored-by: Mounir Dhahri <[email protected]>

artsy-peril · 2025-01-15T09:45:14Z

	Warnings
⚠️	The V2 schema in this PR has breaking changes with Force. Remember to update the Force schema if necessary. Field 'seoExperimentArtists' was removed from object type 'Query' Field 'isInSeoExperiment' was removed from object type 'Artist' Field 'teamName' was removed from object type 'Job' Field 'seoExperimentArtists' was removed from object type 'Viewer' Field 'homeViewTasksSection' was removed from object type 'AckTaskMutationPayload' Field 'homeViewTasksSection' was removed from object type 'DismissTaskMutationPayload'

Generated by 🚫 dangerJS against 142eb4c

olerichter00

Looks great 🌟

olerichter00 · 2025-01-15T09:56:23Z

src/schema/v2/homeView/sectionTypes/QuickLinks.ts

+/*
+ * A section type in the home view is specified declaratively
+ * as a GraphQL object that implements the HomeViewGenericSectionInterface
+ *
+ * Below we will configure its various fields.
+ */


🧹

Suggested change

/*

* A section type in the home view is specified declaratively

* as a GraphQL object that implements the HomeViewGenericSectionInterface

*

* Below we will configure its various fields.

*/

olerichter00 · 2025-01-15T09:56:50Z

src/schema/v2/homeView/sections/QuickLinks.ts

+
+export const QuickLinks: HomeViewSection = {
+  id: "home-view-section-quick-links",
+  // TODO: add ContextModule quickLinks


Co-authored-by: Mounir Dhahri <[email protected]>

MounirDhahri · 2025-01-16T12:41:07Z

src/schema/v2/homeView/sectionTypes/QuickLinks.ts

+    contextScreenOwnerId: {
+      type: GraphQLString,
+      description: "The owner ID for the context module",


@anandaroop @dblandin what do you think about this?

What is this used for? The only specific value I see currently is "/collect?price_range=*-1000" which doesn't read as an ID to me.

Is this needed value need for the analytics support?

yes, that's what we have in mind. We can also use the href as a payload for the tracking event. it's not clear so far.

To capture the href value, you could use destination_path which we've included in similar events.

artsy/cohesion#525

src/schema/v2/homeView/sectionTypes/QuickLinks.ts

Co-authored-by: Mounir Dhahri <[email protected]>

dblandin

Looking good!

Some high-level feedback:

I like "QuickLinks" as the name of the specific section instance but I wonder if we can abstract the section type a bit more. Maybe "NavigationPills"?

That way, we can potentially have more than one instance of this section type in the schema. For example, maybe later on we have a need for two different section instances (e.g. "ForYouQuickLinks" and "ExploreQuickLinks") and each can be backed by the same "NavigationPills" section type.

What do you think?

dblandin · 2025-01-16T12:56:53Z

src/schema/v2/homeView/sectionTypes/QuickLinks.ts

+    contextScreenOwnerId: {
+      type: GraphQLString,
+      description: "The owner ID for the context module",


What is this used for? The only specific value I see currently is "/collect?price_range=*-1000" which doesn't read as an ID to me.

Is this needed value need for the analytics support?

MounirDhahri · 2025-01-16T13:12:52Z

That's great feedback @dblandin thanks

MounirDhahri · 2025-01-16T13:11:56Z

src/schema/v2/homeView/sectionTypes/QuickLinks.ts

+    contextScreenOwnerId: {
+      type: GraphQLString,
+      description: "The owner ID for the context module",


yes, that's what we have in mind. We can also use the href as a payload for the tracking event. it's not clear so far.

MounirDhahri · 2025-01-16T13:54:35Z

src/schema/v2/homeView/sections/NavigationPills.ts

@@ -2,25 +2,25 @@ import { ContextModule, OwnerType } from "@artsy/cohesion"
 import { HomeViewSection } from "."
 import { HomeViewSectionTypeNames } from "../sectionTypes/names"

-export const QuickLinks: HomeViewSection = {
+export const NavigationPills: HomeViewSection = {


I believe that we don't need to rename this. Renaming the section type should be enough

Hmmm I have the same question I'm asking here: is there a reason you want to keep different names?

Cross-linking this comment: artsy/cohesion#548 (comment)

Yeah, I think NavigationPills is a good name for the section type — but for this section instance I like the idea of calling it something else like QuickLinks. That way we decouple the section type from the specific, concrete section that uses it, as we have for the previous sections mentioned in Devon's linked comment.

This is of greater value for section types that can be re-used and I'm not sure if this one will be, but I think this is a good default for us to follow.

Ah omg, I get it now, thanks everyone for explaining!

MounirDhahri · 2025-01-16T15:19:20Z

src/schema/v2/homeView/sections/NavigationPills.ts

+}
+
+export interface NavigationPill {
+  contextScreenOwnerId?: string | null


action item: to delete

anandaroop

Looking good!

I've just added some thoughts about naming and organization, similar to others' comments.

I'd be in favor of de-coupling the section type (NavigationPills) from the specific section instance (QuickLinks?) even if there isn't an immediately obvious opportunity for re-use of the section type.

The type:

// sectionTypes/NavigationPills.ts

export interface NavigationPill {
  …
}

const NavigationPillType = new GraphQLObjectType<NavigationPill, …>({
  name: "NavigationPill",
  ...
})

export const HomeViewNavigationPillsSectionType = new GraphQLObjectType({
  name: HomeViewSectionTypeNames.HomeViewSectionNavigationPills,
  description: "A selection of navigational pills in the home view",
  ...
})

The instance:

// sections/QuickLinks.ts

export const QuickLinks: HomeViewSection = {
  id: "home-view-section-quick-links",
  type: HomeViewSectionTypeNames.HomeViewSectionNavigationPills,
  …
}

src/schema/v2/homeView/sectionTypes/NavigationPills.ts

anandaroop · 2025-01-16T18:29:13Z

src/schema/v2/homeView/sections/NavigationPills.ts

@@ -2,25 +2,25 @@ import { ContextModule, OwnerType } from "@artsy/cohesion"
 import { HomeViewSection } from "."
 import { HomeViewSectionTypeNames } from "../sectionTypes/names"

-export const QuickLinks: HomeViewSection = {
+export const NavigationPills: HomeViewSection = {


Yeah, I think NavigationPills is a good name for the section type — but for this section instance I like the idea of calling it something else like QuickLinks. That way we decouple the section type from the specific, concrete section that uses it, as we have for the previous sections mentioned in Devon's linked comment.

This is of greater value for section types that can be re-used and I'm not sure if this one will be, but I think this is a good default for us to follow.

anandaroop · 2025-01-16T18:33:53Z

src/schema/v2/homeView/sections/NavigationPills.ts

+export const NavigationPills: HomeViewSection = {
+  id: "home-view-section-navigation-pills",
+  contextModule: ContextModule.quickLinks,
+  ownerType: OwnerType.quickLinks,


question: This ownerType optional attribute is intended for a section's view-all screen — I think that's not actually relevant or required here, right?

anandaroop · 2025-01-16T18:35:24Z

src/schema/v2/homeView/sections/NavigationPills.ts

+  },
+}
+
+export interface NavigationPill {


question: I wonder if it makes more sense for this interface to live over in the NavigationPill section type and be imported here? This interface is very coupled to that section type's response shape.

feat: add quick links section

67f363d

Co-authored-by: Mounir Dhahri <[email protected]>

dariakoko requested review from olerichter00 and MounirDhahri January 15, 2025 09:44

dariakoko self-assigned this Jan 15, 2025

olerichter00 previously approved these changes Jan 15, 2025

View reviewed changes

chore: add tracking

a96cd8c

Co-authored-by: Mounir Dhahri <[email protected]>

artsyit dismissed olerichter00’s stale review via a96cd8c January 15, 2025 14:31

dariakoko and others added 2 commits January 15, 2025 15:41

chore: add tests

9a781b6

Co-authored-by: Mounir Dhahri <[email protected]>

feat: add contextScreenOwnerId and ownerType

6e9b2d9

MounirDhahri reviewed Jan 16, 2025

View reviewed changes

MounirDhahri previously approved these changes Jan 16, 2025

View reviewed changes

src/schema/v2/homeView/sectionTypes/QuickLinks.ts Outdated Show resolved Hide resolved

src/schema/v2/homeView/sectionTypes/QuickLinks.ts Outdated Show resolved Hide resolved

MounirDhahri requested review from dblandin and anandaroop January 16, 2025 12:44

Update src/schema/v2/homeView/sectionTypes/QuickLinks.ts

48a6fbf

Co-authored-by: Mounir Dhahri <[email protected]>

dariakoko dismissed MounirDhahri’s stale review via 48a6fbf January 16, 2025 12:44

Update src/schema/v2/homeView/sectionTypes/QuickLinks.ts

a9ddd2e

Co-authored-by: Mounir Dhahri <[email protected]>

dblandin reviewed Jan 16, 2025

View reviewed changes

dariakoko added 2 commits January 16, 2025 14:30

chore: rename QiockLinks to NavigationPills

65680d9

chore: rename section id

b11c956

dariakoko mentioned this pull request Jan 16, 2025

chore: rename quickLinks to navigationPills artsy/cohesion#548

Closed

3 tasks

MounirDhahri reviewed Jan 16, 2025

View reviewed changes

anandaroop reviewed Jan 16, 2025

View reviewed changes

dariakoko added 2 commits January 17, 2025 11:20

chore: rename

7dc80e8

chore: remove contextScreenOwnerId

142eb4c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ONYX-1479): add quick links section #6354

feat(ONYX-1479): add quick links section #6354

dariakoko commented Jan 15, 2025

artsy-peril bot commented Jan 15, 2025 •

edited

Loading

olerichter00 left a comment

olerichter00 Jan 15, 2025

olerichter00 Jan 15, 2025

MounirDhahri Jan 16, 2025

dblandin Jan 16, 2025

MounirDhahri Jan 16, 2025

dblandin Jan 16, 2025

dblandin left a comment

dblandin Jan 16, 2025

MounirDhahri commented Jan 16, 2025

MounirDhahri Jan 16, 2025

MounirDhahri Jan 16, 2025

dariakoko Jan 16, 2025

dblandin Jan 16, 2025

anandaroop Jan 16, 2025

dariakoko Jan 17, 2025

MounirDhahri Jan 16, 2025

anandaroop left a comment •

edited

Loading

anandaroop Jan 16, 2025

anandaroop Jan 16, 2025

anandaroop Jan 16, 2025

feat(ONYX-1479): add quick links section #6354

Are you sure you want to change the base?

feat(ONYX-1479): add quick links section #6354

Conversation

dariakoko commented Jan 15, 2025

artsy-peril bot commented Jan 15, 2025 • edited Loading

olerichter00 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

dblandin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounirDhahri commented Jan 16, 2025

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

anandaroop left a comment • 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

artsy-peril bot commented Jan 15, 2025 •

edited

Loading

anandaroop left a comment •

edited

Loading