Adds collection artwork preview component #2015

ashleyjelks · 2019-12-17T14:36:44Z

This PR adds the artwork preview grid to the collections view.

We have a mob pairing session scheduled to work on adding tests, but I think this PR is at least in a good state to have the code reviewed for the component itself.

#skip_new_tests

pepopowitz

It looks like the actual CollectionArtworkPreview component is not included in this PR. Is that accidental, or because it is a component that already existed?

pepopowitz · 2019-12-17T15:26:28Z

src/lib/Scenes/Collection/Screens/CollectionArtworkPreview.tsx

+    if (!collection) {
+      return null
+    }
+    const artworks = collection.artworks.edges.map(({ node }) => node)


Should we use the get util against this long chain of properties, to avoid app crashes if for some reason something came back from metaphysics funny?

pepopowitz

Looks good, but I think the snapshot tests are not capturing what you're expecting them to capture. My other two comments are trivial, and I'm not expecting them to be addressed.

pepopowitz · 2019-12-19T16:44:47Z

src/lib/Scenes/Collection/Screens/CollectionArtworkPreview.tsx

+
+export class CollectionArtworkPreview extends React.Component<Props> {
+  render() {
+    let artworks


This is a total nit, and not something I'm expecting to change as part of this PR. Having a variable defined with let at the top of a function gives me the impression that there's going to be some conditional logic to decide what to set it to. That isn't the case here, so I'd expect the declaration to happen on line 20, where it is set, like

const artworks = collectionArtworks.map....

Declaring it as a let at the top just kind of leaves my brain hanging...it's something I have to keep track of as I read the function, to know when and how it's set.

pepopowitz · 2019-12-19T16:53:25Z

...ib/Scenes/Collection/Screens/__tests__/__snapshots__/CollectionArtworkPreview-tests.tsx.snap

@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`CollectionArtworkPreview renders properly 1`] = `null`;


This snapshot is null - it looks like we didn't capture what we intended to capture.

pepopowitz · 2019-12-19T16:55:45Z

src/lib/Scenes/Collection/Screens/__tests__/__snapshots__/CollectionHeader-tests.tsx.snap

-  </Text>,
-]
-`;
+exports[`renders a snapshot of the collection header 1`] = `"<view mb=\\"2\\"><aropaqueimageview imageurl=\\"http://imageuploadedbymarketingteam.jpg\\" height=\\"204\\" width=\\"750\\"></aropaqueimageview></view>"`;


I don't think this snapshot captures what we intended to capture, either. From the CollectionHeader component, I'd expect this snapshot to include the title, which I don't see.

pepopowitz · 2019-12-19T17:01:10Z

src/lib/Scenes/Collection/Screens/CollectionArtworkPreview.tsx

+    if (!collection) {
+      return null
+    }
+    const collectionArtworks = get(collection.artworks, artwork => artwork.edges, [])


Does the get helper err out if the first argument is undefined? If so, or if you don't feel like looking, a quick way to avoid that happening here would be to move .artworks to the second argument, like this:

Suggested change

const collectionArtworks = get(collection.artworks, artwork => artwork.edges, [])

const collectionArtworks = get(collection, collection => collection.artworks.artwork.edges, [])

anandaroop

Only comment from me is the thing I noted in Slack yesterday…

anandaroop · 2019-12-19T18:41:51Z

src/lib/Scenes/Collection/Screens/CollectionArtworkPreview.tsx

+export const CollectionArtworkPreviewContainer = createFragmentContainer(CollectionArtworkPreview, {
+  collection: graphql`
+    fragment CollectionArtworkPreview_collection on MarketingCollection {
+      artworks: artworksConnection(sort: "-merchandisability", first: 6) {


Should be -decayed_merch n'est-ce pas? Slack convo here...

Yes, but I want to follow up with this in a sep PR bc it requires a change in Reaction and Emission and for now I think the data should be consistent.

pepopowitz

I still don't think these snapshot tests are functioning correctly. The snapshots themselves don't contain any details about the components that are being rendered.

pepopowitz · 2019-12-20T17:32:18Z

src/lib/Scenes/Collection/Screens/__tests__/__snapshots__/CollectionHeader-tests.tsx.snap

-  </Text>,
-]
-`;
+exports[`renders properly 1`] = `"<view mb=\\"2\\"><aropaqueimageview imageurl=\\"http://imageuploadedbymarketingteam.jpg\\" height=\\"204\\" width=\\"750\\"></aropaqueimageview></view>"`;


This looks different than before, but I still don't think it's capturing what you intended it to capture.

pepopowitz · 2019-12-20T17:32:38Z

...ib/Scenes/Collection/Screens/__tests__/__snapshots__/CollectionArtworkPreview-tests.tsx.snap

@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`renders properly 1`] = `"<view mb=\\"4\\" width=\\"100%\\"></view>"`;


This looks different than before, but I still don't think it's capturing what you intended it to capture.

So this new snapshot is testing the Container and that the relay mock query generates the expected underlying view component. If you compare a similar snapshot in the Show Artworks preview, it's compiling to the same underlying view:

emission/src/lib/Components/Show/__tests__/__snapshots__/ShowArtworksPreview-tests.tsx.snap

Line 3 in 3445422

exports[`renders properly 1`] = `"<text font-family=\\"ReactNativeAGaramondPro-Regular\\" font-size=\\"26px\\" lineheight=\\"32px\\" mt=\\"2\\" mb=\\"3\\">All works</text>"`;

What do you think?

I don't think it's a test worth keeping, if that's what the snapshot looks like. That's not going to catch any changes to the underlying component. I'd ditch the test, or find a way to test the underlying component itself.

I'm going to disable the test for now and follow up with @ds300 or @ashfurrow when they return next week from the break. In the meantime I think this PR is safe to merge!

xtina-starr · 2019-12-20T22:02:03Z

src/lib/Scenes/Collection/Screens/__tests__/CollectionArtworkPreview-tests.tsx

+
+it("renders properly", async () => {
+  const tree = await renderRelayTree({
+    Component: (props: any) => <CollectionArtworkPreview collection={props} {...props} />,


Do you we need the typing here? If so, it might be worth providing the exact type. CollectionArtworkPreview_collection I think???

mbilokonsky

I'll defer to @pepopowitz on this as he's a lot more familiar with the codebase, chiming in now cuz I keep seeing this notification - sorry I didn't leave this comment before the break, see y'all in 2020!

artsyit · 2020-01-02T13:32:38Z

🚀 PR was released in v1.20.6 🚀

ashleyjelks requested a review from pepopowitz December 17, 2019 14:36

ashleyjelks assigned xtina-starr Dec 17, 2019

artsy-peril bot added Jira Synced Version: Patch labels Dec 17, 2019

pepopowitz reviewed Dec 17, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

adds collection artwork preview component

53a35b3

ashleyjelks force-pushed the artwork-preview branch from 4ea039a to 53a35b3 Compare December 17, 2019 15:11

pepopowitz reviewed Dec 17, 2019

View reviewed changes

ashleyjelks force-pushed the artwork-preview branch from 6f5b020 to 1d4f4dc Compare December 17, 2019 16:58

updates collection tests

14c0859

ashleyjelks force-pushed the artwork-preview branch from 1d4f4dc to 14c0859 Compare December 17, 2019 17:17

ashleyjelks requested a review from pepopowitz December 18, 2019 10:20

ashleyjelks added 2 commits December 19, 2019 13:04

updates tests in collection components

b0a5947

merge latest master and resolve query map conflict

dc47e5e

ashleyjelks force-pushed the artwork-preview branch from d5d1769 to dc47e5e Compare December 19, 2019 12:18

ashleyjelks requested review from anandaroop and mbilokonsky December 19, 2019 12:38

pepopowitz suggested changes Dec 19, 2019

View reviewed changes

anandaroop reviewed Dec 19, 2019

View reviewed changes

ashleyjelks added 2 commits December 20, 2019 14:27

updates missing snapshots

bfe1035

renames vars and calls get earlier

70b8cd2

ashleyjelks requested a review from pepopowitz December 20, 2019 13:34

adds a test for header title

b17054f

xtina-starr approved these changes Dec 20, 2019

View reviewed changes

pepopowitz suggested changes Dec 20, 2019

View reviewed changes

xtina-starr reviewed Dec 20, 2019

View reviewed changes

mbilokonsky reviewed Dec 28, 2019

View reviewed changes

disables snapshot

9c92c2f

ashleyjelks merged commit 99a838a into master Jan 2, 2020

ashleyjelks deleted the artwork-preview branch January 2, 2020 13:24

artsyit added the released label Jan 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds collection artwork preview component #2015

Adds collection artwork preview component #2015

ashleyjelks commented Dec 17, 2019 •

edited

Loading

pepopowitz left a comment

This comment has been minimized.

pepopowitz Dec 17, 2019

pepopowitz left a comment

pepopowitz Dec 19, 2019

pepopowitz Dec 19, 2019

pepopowitz Dec 19, 2019

pepopowitz Dec 19, 2019

anandaroop left a comment

anandaroop Dec 19, 2019

ashleyjelks Dec 20, 2019

pepopowitz left a comment

pepopowitz Dec 20, 2019

pepopowitz Dec 20, 2019

ashleyjelks Dec 20, 2019

pepopowitz Dec 20, 2019

ashleyjelks Jan 2, 2020

xtina-starr Dec 20, 2019

mbilokonsky left a comment

artsyit commented Jan 2, 2020

		@@ -0,0 +1,3 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`CollectionArtworkPreview renders properly 1`] = `null`;

	const collectionArtworks = get(collection.artworks, artwork => artwork.edges, [])
	const collectionArtworks = get(collection, collection => collection.artworks.artwork.edges, [])

		@@ -0,0 +1,3 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`renders properly 1`] = `"<view mb=\\"4\\" width=\\"100%\\"></view>"`;

Adds collection artwork preview component #2015

Adds collection artwork preview component #2015

Conversation

ashleyjelks commented Dec 17, 2019 • edited Loading

pepopowitz left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

pepopowitz 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

anandaroop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepopowitz 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

mbilokonsky left a comment

Choose a reason for hiding this comment

artsyit commented Jan 2, 2020

ashleyjelks commented Dec 17, 2019 •

edited

Loading