[DONT MERGE] feat: load limited-time offer signals on the client side #14897
Conversation
|
#1311 Bundle Size — 8.96MiB (+0.09%).cd42dab(current) vs bfdc630 main#1307(baseline) Warning Bundle contains 14 duplicate packages – View duplicate packages Bundle metrics
|
| Current #1311 |
Baseline #1307 |
|
|---|---|---|
 Initial JS |
3.65MiB(+0.02%) |
3.65MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
74.36% |
41.25% |
| Chunks |
103 |
103 |
| Assets |
106 |
106 |
| Modules |
5840(+0.07%) |
5836 |
| Duplicate Modules |
530 |
530 |
| Duplicate Code |
4.03% |
4.03% |
| Packages |
266 |
266 |
| Duplicate Packages |
13 |
13 |
Bundle size by type 1 change 1 regression
| Current #1311 |
Baseline #1307 |
|
|---|---|---|
| JS |
8.82MiB (+0.09%) |
8.81MiB |
| Other |
143.36KiB |
143.36KiB |
Bundle analysis report Branch review-app-client-side-collector... Project dashboard
Generated by RelativeCI Documentation Report issue
| ) | ||
| } | ||
|
|
||
| const SaleMessageFragmentContainer = createFragmentContainer(SaleMessage, { |
There was a problem hiding this comment.
#nonblocking and harmless, but would def encourage all new relay code to be written using hooks:
const data = useFragment(...)Its a bit nicer from an import perspective too; can do import { SaleMessage } from vs having to mention fragment containers.
There was a problem hiding this comment.
Good to know! I was confused about the best practices doc that recommends relay containers.
| relayEnvironment: mockEnvironment, | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
#non-blocking (but recommended): Rather than mocking out all of this stuff, can simply test the fragment via our relay testing tools and skip all of the qury renderer wiring (which historically we typically don't test):
const { renderWithRelay } = setupTestWrapperTL({
Component: SaleMessageFragmentContainer
query: ...
})
it('works', () => {
renderWithRelay(someFixture)
expect(screen.getByText("foo")).toBeOnScreen()
})There was a problem hiding this comment.
Yeah I intentionally wanted to test the query renderers (vs. the fragment containers), which seems to be not yet supported, to cover the 2-step rendering, e.g. server renders signal A and after fetching data on the clientside, replaces signal A with signal B.
| ) | ||
|
|
||
| act(() => { | ||
| mockEnvironment.mock.resolveMostRecentOperation(operation => |
There was a problem hiding this comment.
This allows us to skip manually mocking both the test environment and the relay operation; all of those things happen inside of our test tools automatically.
| signal_label: artwork.collectorSignals | ||
| ? getSignalLabel(artwork.collectorSignals) | ||
| : "", | ||
| signal_label: getSignalLabel(signals?.[artwork.internalID] ?? []), |
There was a problem hiding this comment.
Would this populate the data when the PrimaryLabelLine component is not rendered with this ArtworkSidebarCommercialButtons component? I think we might still need a way to use the backend sourced signals. But I could be missing something here.
This depends on MP change that removes partner offers from primary labels. Primary label should only consist of unauthenticated data.
Now we have to consider partner offer (that's loaded on the client side) when displaying "increased interest" and "curtors' pick" badges (that can be loaded on the server and cached), it may visually look better moving the limited-time offer back to the first line, with the badges. While there will be some content shift, we can preserve the original collector signals design (vs. partially porting over the app design).
rquartararo
force-pushed
the
review-app-client-side-collector-signals
branch
from
601b617 to
cd42dab
Compare
The type of this PR is: Feat
DO NOT MERGE YET. This depends on artsy/metaphysics#6220, and we might want to include tracking changes.
This PR solves EMI-2161
Description
In the previous attempt, we tried a few design options to display the limited-time offer signals on the client-side. We then realized the signal has to be considered together with other badge signals (i.e. "increased interest" and "curators' pick") because we only want to display at most one. So we decided to switch back to the original signals design and let client-side loaded signals, if any, dynamically replace server-side rendered content. We now think it's acceptable to have content shift for this particular data which is relatively lower volume.
This is an example that the client-side loaded limited-time offer signal replaces an increased interest badge in-place. We can see the offered price also replaces the listed price shortly after page loads, as well as the timer and save state pops in.
Recording
This is an example when without other badges, the limited-time offer badge is inserted which causes some content shift.
Recording
cc @artsy/emerald-devs