[WIP] refactor: load limited-time offer signal on the client side #14807

starsirius · 2024-11-05T21:38:50Z

The type of this PR is: Refactor

This PR solves EMI-2161

Opening a draft PR from the spike. Will need polish and tests, and I realized there are more to be discussed/done, but would like to align on overall direction. 🙏

Description

This loads the partner offer collector signal on the client-side, with the goal of being able to cache artwork grids more comprehensively. It follows the migration pattern by moving the data needing authentication from FragmentContainer to a deferred QueryRenderer.

It's more nuanced than a straight migration because the authenticated data can be displayed in-between server-side rendered content, and "inserting" the data on the client side after page renders can lead to content shift. In particular for limited-time offer signals, there 3 pieces of information:

The "Limited-time offer" badge
Partner offered price that replaces the publicly listed price
Partner offer expiration

What about the badge that's in the middle of an artwork card?

To avoid content shift for the badge, we agreed on migrating to the new artwork card design on app which combines the badge into the fifth line.

How about the final price that we don't know until fetching partner offers?

To support rendering the client-side loaded data, specifically for partner offered price that replaces the listed price, I spiked on 2 approaches and the team agreed on option 1, simply overwriting server-rendered data when client-side data come back. See below:

Slack discussion and recordings about the 2 options

Option 1 (review app): Render publicly listed price on the backend and dynamically overwrite it after fetching partner offer on the client side.
Option 2 (review app): Defer rendering entire pricing until partner offer is fetched on the client side.

Ah, and I realized there are more... 😓

The limited-time offer signal competes with the "increased interest" and "curator's pick", meaning that an artwork can have multiple badges and we'll only show one, based on defined precedence, in which the limited-time offer is the highest. I guess I'll need to start a new discussion about it...

@artsy/emerald-devs

starsirius · 2024-11-05T21:44:10Z

src/Components/Artwork/Details/Details.tsx

-}
+const LINE_HEIGHT = 22
+const NUM_OF_LINES = 5
+const CONTAINER_HEIGHT = LINE_HEIGHT * NUM_OF_LINES


The current implementation requires logic to make sure we have consistent number of lines (e.g. if there is no badge, we will pad with an empty line). Since we agreed on fixed 5 lines, I feel it's easier to set a fixed container height and just add lines to it, without maintaining the # of lines logic which gets a bit tricky with client-side loaded content.

starsirius · 2024-11-05T21:45:52Z

src/Components/Artwork/Details/PartnerOfferLine.tsx

+  )
+}
+
+const PartnerOfferLine: React.FC<PartnerOfferLineProps> = ({ artwork }) => {


This is the combined fifth line that's now fetched and rendered on the client side.

starsirius · 2024-11-05T21:49:55Z

src/Components/Artwork/Details/PartnerOfferedPrice.tsx

+  fallback: object
+}
+
+const PartnerOfferedPrice: React.FC<PartnerOfferedPriceProps> = ({


This is the artwork price line which takes the server-side rendered prices (publicly listed price) as the placeholder and dynamically overwrite it, if there is a partner offer fetched on the client side.

I think I'll need to rename this component to make it clear.

mzikherman

This looks great! Fairly straightforward and nothing 'weird' - good and simple.

I think there's the primaryLabel thing which is what you're referring to in terms of determining precedence to only display one label. My initial thought is that we should be ok w/ sometimes displaying two labels, since we dont know if the offer label exists in the context of caching. Perhaps that means the design needs to change (partner offer collector signal represented differently)?

mzikherman · 2024-11-05T22:27:15Z

src/Components/Artwork/Details/Details.tsx

@@ -430,12 +403,6 @@ export const DetailsFragmentContainer = createFragmentContainer(Details, {
          registrationEndsAt
          onlineBiddingExtended
        }
-        partnerOffer {


Hooray!

I wonder if primaryLabel needs to also be removed here (or updated in MP), as I think it can return the personalized 'offer' label. Or is this perhaps what you refer to in the PR description about the precedence of only displaying one label at most?

Yeah, I decided to remove partner offer from the primary label field for that reason. After artsy/metaphysics#6220, the primary label will be fully cacheable. I think it's also more aligned with the pattern that separates authenticated data out from non-auth data.

I shared more context on Slack.

Cool - makes sense. I think you're right that schema-wise - of course it's a case-by-case basis - but not mixing authenticated and cacheable data as part of the resolving of the same field (in this case collectorSignals under the Artwork type) - is something we should be doing.

I think collector signals might be the only case of that really (🤞), as typically an entire field would be either authenticated or not - and not a more generic-y mix of both as collector signals is.

mzikherman · 2024-11-05T22:37:16Z

src/Components/Artwork/Details/PartnerOfferedPrice.tsx

+      placeholder={<SaleMessage artwork={artwork} />}
+      variables={{ id }}
+      render={({ error, props }) => {
+        const publicPrice = <SaleMessage artwork={artwork} />


mzikherman · 2024-11-05T22:38:47Z

src/Components/Artwork/Details/PartnerOfferedPrice.tsx

+        }
+
+        return (
+          <PartnerOfferedPriceFragmentContainer


Can maybe include a subtle/nice little 'pulse' animation like in

force/src/Apps/Artwork/Components/ArtworkSidebar/ArtworkSidebarCurrentBidInfo.tsx

Line 20 in 4d6f44a

const pulse = keyframes`

mzikherman · 2024-11-05T22:58:37Z

The limited-time offer signal competes with the "increased interest" and "curator's pick", meaning that an artwork can have multiple badges and we'll only show one, based on defined precedence, in which the limited-time offer is the highest. I guess I'll need to start a new discussion about it...

My initial thought is that we should be ok w/ sometimes displaying two labels, since we dont know if the offer label exists in the context of caching. Perhaps that means the design needs to change (partner offer collector signal represented differently)?

Maybe another way of thinking about it schema/product/design-wise, is that 'collector signals' are always public, cached, show up how they show up - the extra line. You know before rendering if that line / any works in a rail or grid will have them. And then the 'offer'/user-specific messaging has a different appearance entirely, maybe isn't even called a 'collector signal' in our schema and is modeled differently, and is also allowed to show up in addition to the regular public collector signal (maybe as a 'badge' in the corner of the image - not sure the best design here). Do you know if there are any plans for other personalized signals/messaging? That may help inform direction.

relativeci · 2024-11-11T15:36:46Z

#943 Bundle Size — 9.51MiB (-0.35%).

c172c74(current) vs ffe4f84 main#484(baseline)

Important

Bundle introduced 1 and removed 4 duplicate packages – View changed duplicate packages

Warning

Bundle introduced 3 new packages: web-vitals, @sentry-internal/browser-utils, stylis – View changed packages

Bundle metrics

9 changes

1 regression

3 improvements

	Current #943	Baseline #484
Initial JS	`3.83MiB`(`-3.07%`)	`3.95MiB`
Initial CSS	`0B`	`0B`
Cache Invalidation	`84.09%`	`2.04%`
Chunks	`140`(`-2.1%`)	`143`
Assets	`144`(`-1.37%`)	`146`
Modules	`5671`(`+0.6%`)	`5637`
Duplicate Modules	`472`(`+3.74%`)	`455`
Duplicate Code	`6.18%`(`+5.1%`)	`5.88%`
Packages	`282`(`-3.09%`)	`291`
Duplicate Packages	`39`(`-7.14%`)	`42`

Bundle size by type

2 changes

2 improvements

	Current #943	Baseline #484
JS	`9.28MiB` (`-0.35%`)	`9.31MiB`
Other	`237.37KiB` (`-0.2%`)	`237.84KiB`

Bundle analysis report Branch review-app-client-side-offer-sig... Project dashboard

^{Generated by RelativeCI Documentation Report issue}

starsirius · 2024-11-20T01:50:08Z

We decided to take a different route and this PR got too big. I'm going to close it and open a new one. Thank you all for the patience! 🙏

artsy-peril bot added the Jira Synced label Nov 5, 2024

starsirius commented Nov 5, 2024

View reviewed changes

starsirius self-assigned this Nov 5, 2024

starsirius requested review from rquartararo, mzikherman and damassi November 5, 2024 21:50

mzikherman previously approved these changes Nov 5, 2024

View reviewed changes

starsirius added 8 commits November 11, 2024 10:28

feat: lazy load partner offer expiration

5829249

feat: remove partner offer from primary label display

ea6bec0

feat: reposition partner offer line; display empty line as placeholder

5972880

feat: remove fetching partner offer and offered price display

c3dc858

fix: only show bid timer line on auction artworks

be716d8

refactor: use fixed height rather than fixed number of lines

9553f7f

fix: do not render partner offer line with no offer

dc94d6b

feat: overwrite public pricing after fetching offers

c172c74

starsirius dismissed mzikherman’s stale review via 6f75eb5 November 11, 2024 15:30

starsirius force-pushed the review-app-client-side-offer-signal-overwrite branch from 1064829 to 6f75eb5 Compare November 11, 2024 15:30

starsirius force-pushed the review-app-client-side-offer-signal-overwrite branch from a4755f2 to c172c74 Compare November 20, 2024 01:47

starsirius closed this Nov 20, 2024

This was referenced Nov 22, 2024

[DONT MERGE] feat: load limited-time offer signals on the client side #14897

Draft

[DONT MERGE] feat: remove partner offer from primary label artsy/metaphysics#6220

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] refactor: load limited-time offer signal on the client side #14807

[WIP] refactor: load limited-time offer signal on the client side #14807

starsirius commented Nov 5, 2024 •

edited

Loading

starsirius Nov 5, 2024

starsirius Nov 5, 2024

starsirius Nov 5, 2024

mzikherman left a comment

mzikherman Nov 5, 2024

starsirius Nov 12, 2024

mzikherman Nov 12, 2024

mzikherman Nov 5, 2024

mzikherman Nov 5, 2024

mzikherman commented Nov 5, 2024 •

edited

Loading

relativeci bot commented Nov 11, 2024 •

edited

Loading

starsirius commented Nov 20, 2024

[WIP] refactor: load limited-time offer signal on the client side #14807

[WIP] refactor: load limited-time offer signal on the client side #14807

Conversation

starsirius commented Nov 5, 2024 • edited Loading

Description

What about the badge that's in the middle of an artwork card?

How about the final price that we don't know until fetching partner offers?

Ah, and I realized there are more... 😓

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzikherman 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

mzikherman commented Nov 5, 2024 • edited Loading

relativeci bot commented Nov 11, 2024 • edited Loading

#943 Bundle Size — 9.51MiB (-0.35%).

starsirius commented Nov 20, 2024

starsirius commented Nov 5, 2024 •

edited

Loading

mzikherman commented Nov 5, 2024 •

edited

Loading

relativeci bot commented Nov 11, 2024 •

edited

Loading