feat: move save button components into query renderers #14533

mzikherman · 2024-09-20T20:42:43Z

This is a bit naive - literally taking all three save button implementations (which are all live / fragment was being requested in our standard Artwork component) - removing their fragments from the parent queries - and replacing the FragmentContainer invocations with QueryRenderers. One actual functionality change which I'll comment on inline.

Nevertheless... it all continues to work! The correct saved state (as well as lot watched count - collector signal which piggybacks onto this saved artwork component - why isn't it is own component? that could be requested + cached up front) 'pops in' momentarily after the main grid query succeeds + renders - it feels totally fine!

General functionality - the save button works when you toggle it on/off, and the toast which includes the 'List' modal popup - also continues to work.

The only personalized snippets remaining in the main filter/grid query:

partner offer collector signal - this is N+1 as well
'followed artists' aggregations for the sidebar

mzikherman · 2024-09-20T20:45:15Z

src/Components/Artwork/FlatGridItem.tsx

@@ -25,7 +23,7 @@ const FlatGridItem: React.FC<FlatGridItemProps> = ({ artwork, onClick }) => {
  const { user } = useSystemContext()
  const isTeam = userIsTeam(user)
  const { containerProps, isSaveButtonVisible } = useSaveButton({
-    isSaved: !!artwork.isSaved,
+    isSaved: false,


This was an issue. isSaved was being requested outside of the save button context, and right in this component. It was passed to the useSaveButton hook - which effectively sets isHover: true - thus letting the old-school deprecated button show up always if you've saved the work (even w/o hovering).

With this change, the 'saved' status doesn't show up unless you hover.

Seems relatively minor to me 🙏

relativeci · 2024-09-20T20:48:13Z

#427 Bundle Size — 9.55MiB (-0.98%).

0f8bcc4(current) vs 96291e1 main#425(baseline)

Warning

Bundle contains 45 duplicate packages – View duplicate packages

Bundle metrics

4 changes

1 improvement

	Current #427	Baseline #425
Initial JS	`3.95MiB`(`-0.68%`)	`3.98MiB`
Initial CSS	`0B`	`0B`
Cache Invalidation	`47.47%`	`0%`
Chunks	`143`	`143`
Assets	`146`	`146`
Modules	`5637`(`+0.05%`)	`5634`
Duplicate Modules	`455`	`455`
Duplicate Code	`5.88%`(`+0.34%`)	`5.86%`
Packages	`291`	`291`
Duplicate Packages	`42`	`42`

Bundle size by type

2 changes

1 regression

1 improvement

	Current #427	Baseline #425
JS	`9.32MiB` (`-1.05%`)	`9.42MiB`
Other	`237.84KiB` (`+1.99%`)	`233.2KiB`

Bundle analysis report Branch saves-list-modal-into-a-renderer Project dashboard

^{Generated by RelativeCI Documentation Report issue}

mzikherman · 2024-09-21T03:31:24Z

src/Apps/ViewingRoom/Routes/Works/__tests__/ViewingRoomWorksRoute.jest.tsx

@@ -167,7 +167,6 @@ const ViewingRoomWorksRouteFixture: ViewingRoomWorksRoute_Test_Query$rawResponse
            date: "2015",
            additionalInformation: "some description",
            href: "/artwork/bill-miles-beep-beep",
-            artistNames: "Artist Name",


not sure why the typescript compiler started having issues with all these fields...

mzikherman · 2024-09-21T03:32:17Z

src/Components/Artwork/Details/Details.tsx

@@ -366,17 +366,17 @@ export const Details: React.FC<DetailsProps> = ({

    if (!saveOnlyToDefaultList) {
      return (
-        <SaveArtworkToListsButtonFragmentContainer
+        <SaveArtworkToListsButtonQueryRenderer


This is the main 'type' of change, continue to prop-drill this analytics prop, but instead of passing in the masked fragment via the artwork prop, pass in the artwork id to the query renderer.

mzikherman · 2024-09-21T03:32:49Z

src/Components/Artwork/Details/Details.tsx

@@ -605,8 +605,6 @@ export const DetailsFragmentContainer = createFragmentContainer(Details, {
      }
      ...PrimaryLabelLine_artwork
      ...BidTimerLine_artwork
-      ...SaveButton_artwork


This is big - removing these fragments from the standard Artwork component query (used in rails + grids).

mzikherman · 2024-09-21T03:37:08Z

src/Components/Artwork/FlatGridItem.tsx

@@ -95,9 +93,9 @@ const FlatGridItem: React.FC<FlatGridItemProps> = ({ artwork, onClick }) => {
          )}

          {isSaveButtonVisible && (
-            <DeprecatedSaveButtonFragmentContainer
+            <DeprecatedSaveButtonQueryRenderer


Same thing as above, swapping out fragment container -> query renderer

mzikherman · 2024-09-21T03:37:25Z

src/Components/Artwork/FlatGridItem.tsx

@@ -133,7 +131,6 @@ export const FlatGridItemFragmentContainer = createFragmentContainer(
          @arguments(
            includeConsignmentSubmission: $includeConsignmentSubmission
          )
-        ...DeprecatedSaveButton_artwork


Removing the last of these personalized 'save' bits from the overall Artwork query.

mzikherman · 2024-09-21T03:38:32Z

src/Components/Artwork/SaveButton/SaveArtworkToListsButton.tsx

+        }
+      `}
+      placeholder={
+        // TODO: This should be a better placeholder, rather than reusing `SaveButtonBase`


Bit of a hack, trying to reuse the main React component (which is typed with Relay data) as the placeholder

damassi · 2024-09-23T16:53:57Z

src/Components/Artwork/SaveButton/DeprecatedSaveButton.tsx

+          return (
+            <Clickable onClick={() => {}}>
+              <HeartStrokeIcon fill="white100" width={24} height={24} />
+            </Clickable>


This makes sense!

damassi

👌

mzikherman requested review from damassi and a team September 20, 2024 20:42

mzikherman self-assigned this Sep 20, 2024

mzikherman requested a review from joeyAghion September 20, 2024 20:43

mzikherman commented Sep 20, 2024

View reviewed changes

mzikherman force-pushed the saves-list-modal-into-a-renderer branch from 043560d to 9d24d12 Compare September 21, 2024 03:30

mzikherman commented Sep 21, 2024

View reviewed changes

mzikherman force-pushed the saves-list-modal-into-a-renderer branch from 9d24d12 to a0d0e94 Compare September 21, 2024 03:36

mzikherman commented Sep 21, 2024

View reviewed changes

feat: move save button components into query renderers

0bc4350

mzikherman force-pushed the saves-list-modal-into-a-renderer branch from a0d0e94 to 0bc4350 Compare September 21, 2024 03:42

mzikherman added the performance label Sep 21, 2024

damassi reviewed Sep 23, 2024

View reviewed changes

damassi previously approved these changes Sep 23, 2024

View reviewed changes

damassi added the Merge On Green label Sep 23, 2024

fix: tests

0f8bcc4

damassi dismissed their stale review via 0f8bcc4 September 23, 2024 17:45

damassi force-pushed the saves-list-modal-into-a-renderer branch from 11f936f to 0f8bcc4 Compare September 23, 2024 17:45

artsy-peril bot merged commit 955a170 into main Sep 23, 2024
13 checks passed

artsy-peril bot deleted the saves-list-modal-into-a-renderer branch September 23, 2024 17:59

artsy-peril bot mentioned this pull request Sep 23, 2024

Deploy #14536

Merged

starsirius mentioned this pull request Nov 5, 2024

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

Closed

mzikherman mentioned this pull request Nov 8, 2024

fix: remove @cacheable from non cacheable requests artsy/eigen#11070

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move save button components into query renderers #14533

feat: move save button components into query renderers #14533

mzikherman commented Sep 20, 2024 •

edited

Loading

mzikherman Sep 20, 2024

relativeci bot commented Sep 20, 2024 •

edited

Loading

mzikherman Sep 21, 2024

mzikherman Sep 21, 2024

mzikherman Sep 21, 2024

mzikherman Sep 21, 2024

mzikherman Sep 21, 2024

mzikherman Sep 21, 2024

damassi Sep 23, 2024

damassi left a comment

feat: move save button components into query renderers #14533

feat: move save button components into query renderers #14533

Conversation

mzikherman commented Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

relativeci bot commented Sep 20, 2024 • edited Loading

#427 Bundle Size — 9.55MiB (-0.98%).

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

damassi left a comment

Choose a reason for hiding this comment

mzikherman commented Sep 20, 2024 •

edited

Loading

relativeci bot commented Sep 20, 2024 •

edited

Loading