chore: Clean up artwork rail and extact sub components #10802

olerichter00 · 2024-09-18T10:10:27Z

This PR resolves ONYX-1290

Previous Refactoring PR: chore: Simplify Previously Sold on Artsy artworks rail #10801

Description

This PR:

cleans up the artwork rail component and its sub-components,
creates the feature flag ARenableArtworkRailRedesignImageAspectRatio,
creates a new artwork rail image component that can be used for the new aspect ratio (LegacyArtworkRailCardImage vs ArtworkRailCardImage).

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.

olerichter00 · 2024-09-18T10:37:35Z

src/app/Components/ArtworkGrids/ArtworkGridItem.tsx

@@ -533,6 +533,7 @@ export default createFragmentContainer(Artwork, {
      width: { type: "Int" }
    ) {
      ...CreateArtworkAlertModal_artwork
+      ...ContextMenuArtwork_artwork @arguments(width: $width)


Instead of passing the data we want to pass a fragment ref to ContextMenuArtwork.

olerichter00 · 2024-09-18T10:37:59Z

src/app/Components/ArtworkRail/ArtworkRailCard.tsx

@@ -127,29 +107,6 @@ export const ArtworkRailCard: React.FC<ArtworkRailCardProps> = ({
    contextScreen,
  } = restProps

-  const containerWidth = useMemo(() => {


Not needed because we define the container width within the image component.

olerichter00 · 2024-09-18T10:38:25Z

src/app/Components/ArtworkRail/ArtworkRailCard.tsx

+            {enableArtworkRailRedesignImageAspectRatio ? (
+              <ArtworkRailCardImage artwork={artwork} />
+            ) : (
+              <LegacyArtworkRailCardImage artwork={artwork} urgencyTag={urgencyTag} />
+            )}


Rendering the old or new image component depending on the feature flag enableArtworkRailRedesignImageAspectRatio

olerichter00 · 2024-09-18T10:38:57Z

src/app/Components/ArtworkRail/ArtworkRailCardImage.tsx

+  artwork: ArtworkRailCardImage_artwork$key
+}
+
+export const ArtworkRailCardImage: React.FC<ArtworkRailCardImageProps> = ({ ...restProps }) => {


This is the new image component where we can implement the new layout and aspect ratio.

ArtsyOpenSource · 2024-09-18T10:41:34Z

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

Generated by 🚫 dangerJS against 7f6da84

dblandin

Fantastic 🙌🏼

src/app/store/config/features.ts

dblandin · 2024-09-18T10:55:45Z

src/app/Components/ArtworkRail/ArtworkRailCard.tsx

    ...CreateArtworkAlertModal_artwork
+    ...ArtworkRailCardImage_artwork
+    ...LegacyArtworkRailCardImage_artwork
+    ...ContextMenuArtwork_artwork


praise: Awesome 💯

MrSltun

Looks great!! I just added one comment 🌟

MrSltun · 2024-09-18T11:51:34Z

src/app/Components/ArtworkRail/ArtworkRailCard.tsx

+            {enableArtworkRailRedesignImageAspectRatio ? (
+              <ArtworkRailCardImage artwork={artwork} />
+            ) : (
+              <LegacyArtworkRailCardImage artwork={artwork} urgencyTag={urgencyTag} />


This is emerald specific, can we remove the urgency tag since they're supposed to be removed after releasing the new auction signals?

Suggested change

<LegacyArtworkRailCardImage artwork={artwork} urgencyTag={urgencyTag} />

<LegacyArtworkRailCardImage artwork={artwork} urgencyTag={!displayAuctionSignal ? urgencyTag : null} />

This is great! So I can remove all the logic around urgency tags within the artwork rail completely?

since we released the auction signals, yeah feel free to remove the urgency tags logic :)

MounirDhahri

Great one

MounirDhahri · 2024-09-18T12:19:39Z

src/app/Components/ContextMenu/ContextMenuArtworkPreviewCardImage.tsx

+  ARTWORK_RAIL_CARD_IMAGE_HEIGHT,
+  ARTWORK_RAIL_CARD_IMAGE_WIDTH,
+} from "app/Components/ArtworkRail/LegacyArtworkRailCardImage"
+import { OpaqueImageView } from "app/Components/OpaqueImageView2"


This is probably not in the scope of this PR. Maybe in a future PR we can use Image from palette here

chore: Clean up artwork rail and extact sub components

07c0a57

artsy-peril bot added the Jira Synced label Sep 18, 2024

olerichter00 self-assigned this Sep 18, 2024

olerichter00 marked this pull request as draft September 18, 2024 10:12

olerichter00 added 2 commits September 18, 2024 12:23

feature flag and new image component

c123ff7

remove code

8ebbbe0

olerichter00 commented Sep 18, 2024

View reviewed changes

olerichter00 marked this pull request as ready for review September 18, 2024 10:39

olerichter00 requested review from MrSltun, MounirDhahri, dariakoko and dblandin September 18, 2024 10:41

dblandin previously approved these changes Sep 18, 2024

View reviewed changes

rename ff

9a76488

olerichter00 dismissed dblandin’s stale review via 9a76488 September 18, 2024 10:58

MrSltun previously approved these changes Sep 18, 2024

View reviewed changes

MounirDhahri previously approved these changes Sep 18, 2024

View reviewed changes

remove urgency tags

f1ec362

olerichter00 dismissed stale reviews from MounirDhahri and MrSltun via f1ec362 September 18, 2024 13:20

olerichter00 added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Sep 18, 2024

tests

7f6da84

artsy-peril bot merged commit 1aec4b6 into main Sep 18, 2024
7 checks passed

artsy-peril bot deleted the olerichter00/ONYX-1290/clean-up-artwork-rail-and-extact-sub-components branch September 18, 2024 13:40

MounirDhahri mentioned this pull request Sep 24, 2024

chore: separate artwork card meta #10829

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Clean up artwork rail and extact sub components #10802

chore: Clean up artwork rail and extact sub components #10802

olerichter00 commented Sep 18, 2024 •

edited

Loading

olerichter00 Sep 18, 2024

olerichter00 Sep 18, 2024

olerichter00 Sep 18, 2024

olerichter00 Sep 18, 2024

ArtsyOpenSource commented Sep 18, 2024 •

edited

Loading

dblandin left a comment

dblandin Sep 18, 2024

MrSltun left a comment •

edited

Loading

MrSltun Sep 18, 2024

olerichter00 Sep 18, 2024

MrSltun Sep 18, 2024

olerichter00 Sep 18, 2024

MounirDhahri left a comment

MounirDhahri Sep 18, 2024

	<LegacyArtworkRailCardImage artwork={artwork} urgencyTag={urgencyTag} />
	<LegacyArtworkRailCardImage artwork={artwork} urgencyTag={!displayAuctionSignal ? urgencyTag : null} />

chore: Clean up artwork rail and extact sub components #10802

chore: Clean up artwork rail and extact sub components #10802

Conversation

olerichter00 commented Sep 18, 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

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

ArtsyOpenSource commented Sep 18, 2024 • edited Loading

dblandin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSltun 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

Choose a reason for hiding this comment

MounirDhahri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olerichter00 commented Sep 18, 2024 •

edited

Loading

ArtsyOpenSource commented Sep 18, 2024 •

edited

Loading

MrSltun left a comment •

edited

Loading