Collections Artwork Filter Container #2073

ashleyjelks · 2020-02-03T22:28:40Z

This PR combines #2067 and #2068 into a single branch to support building the collections artwork filter container as outlined in FX-1474

Includes:

With Feature Flag Enabled (Modal Visible)

With Feature Flag Disabled (Modal Not Visible)

pepopowitz

I have several questions, but this is really great work. 💪

src/lib/Scenes/Artwork/Components/ArtworkActions.tsx

src/lib/Scenes/Collection/Collection.tsx

src/lib/Components/FilterModal.tsx

pepopowitz · 2020-02-07T20:36:10Z

src/lib/Components/__tests__/FilterModal-tests.tsx

+
+import { FilterModal } from "lib/Components/FilterModal"
+
+it("looks like expected", () => {


A couple more-specific tests would be valuable for this component. Things like...

It only renders when the visible prop is true

It can be closed

pepopowitz · 2020-02-07T20:40:18Z

src/lib/Scenes/Collection/Collection.tsx

      default:
        return null
    }
  }
+  onViewableItemsChanged = ({ viewableItems }) => {


There's kind of a lot going on in this function, and I don't really know what it all is doing. It's possible that this is a common pattern in Emission and I just don't have experience with it, but if that's not the case, is there anything we could do to make it more clear?

Hmm, I don't think the objective is to always have code be pretty clear. The function takes an array of all objects that are currently in the viewport and updates the state if the conditionally rendered component we are looking to show/hide is/is not on the screen. I'm happy to add some comments for added clarity. Would you be interested in pairing on a re-factor?

I actually feel the opposite - the main point of code review, IMO, is to make sure the code is clear enough that anyone without context can understand when they come across it in a few months.

I'm definitely up for pairing to refactor!

Woops! DEFINITELY a typo, I meant I think the objective is to always have code be pretty clear. This is what I get for multitasking!

sweir27

Will echo some of Steve's comments! But this is looking awesome!

sweir27 · 2020-02-10T17:34:55Z

src/lib/Components/FilterModal.tsx

+    /*
+    https://app.zeplin.io/project/5aabc5e0786bbb29b6e3dc7f/screen/5dba03df09bf3f5547162ac9
+
+    Next steps:


#minor but given we've tasked these out in Jira I don't think you need to commit these "Next Steps" comments into code!

pepopowitz

Looks great! Thanks for digging into the test coverage.

sweir27 · 2020-02-11T20:59:48Z

src/lib/Scenes/Collection/Collection.tsx

      default:
        return null
    }
  }
+  onViewableItemsChanged = ({ viewableItems }) => {
+    ;(viewableItems || []).map(viewableItem => {
+      const artworksRenderItem = viewableItem?.item?.type || ""


A #minor comment (feel free to punt until a later PR!) but the variable naming here is a little bit confusing (artworksRenderItem feels a little too specific given we may or may not be handling the artwork filter).

Something like

const itemType = viewableItem?.item?.type || "" const itemIsVisible = viewableItem?.isViewable || false if (itemType === "collectionsArtworks" && itemIsVisible)..

could be clearer!

sweir27 · 2020-02-11T21:00:32Z

Nice work!!!

artsyit · 2020-02-11T21:10:08Z

🚀 PR was released in v1.21.44 🚀

anandaroop

Thanks for setting this up! I finally got a chance to fully read through this.

Looks great, though I did notice a couple of quirks around how we invoke setState — something to revisit next time we're in here.

anandaroop · 2020-02-11T22:11:02Z

src/lib/Components/FilterModal.tsx

+  width: 100%;
+  background-color: white;
+  height: ${({ visible }) => (visible ? "auto" : "0")};
+  padding: ${({ visible }) => (visible ? "20px" : "0")};


Is there a reason for the padding to differ depending on the visibility state?

anandaroop · 2020-02-11T22:18:30Z

src/lib/Components/FilterModal.tsx

+                <ModalInnerView visible={this.state.isComponentMounted}>
+                  <Flex flexDirection="row" justifyContent="space-between">
+                    <Flex alignItems="flex-end" mt={0.5} mb={2}>
+                      <Box onTouchStart={() => this.closeModal()}>


#minor could just be

<Box onTouchStart={this.closeModal}>

since the function is simply being executed without any args.

anandaroop · 2020-02-12T19:44:41Z

src/lib/Scenes/Collection/Collection.tsx

+        return this.setState(_prevState => ({ isArtworkGridVisible: true }))
+      }

+      return this.setState(_prevState => ({ isArtworkGridVisible: false }))


These setState calls seem to be unnecessarily using an updater function (the previous state stored in _prevState is not being used here.)

On the other hand... (see next comment)

anandaroop · 2020-02-12T19:45:57Z

src/lib/Scenes/Collection/Collection.tsx

+    })
+  }
+  handleFilterArtworksModal() {
+    this.setState({ isFilterArtworksModalVisible: !this.state.isFilterArtworksModalVisible })


...This call actually would require the updater function, since the new state does depend on previous state.

So I'd change this as follows:

this.setState((prevState) => ({ isFilterArtworksModalVisible: !prevState.isFilterArtworksModalVisible }))

This fuller explanation from the React docs might help! See the State Updates May Be Asynchronous section under https://reactjs.org/docs/state-and-lifecycle.html

ashleyjelks self-assigned this Feb 3, 2020

artsy-peril bot added the Version: Patch label Feb 3, 2020

ashleyjelks requested review from sweir27 and anandaroop February 3, 2020 22:30

ashleyjelks force-pushed the collections-filter branch from f8aee31 to 6701c9d Compare February 3, 2020 22:30

ashleyjelks requested a review from xtina-starr February 3, 2020 22:30

ashleyjelks changed the title ~~Collections filter~~ [WIP] Collections Artwork Filter Feb 3, 2020

ashleyjelks mentioned this pull request Feb 3, 2020

[WIP] Adds toggling filter button for iOS Collections #2068

Closed

ashleyjelks force-pushed the collections-filter branch from 6701c9d to a972e06 Compare February 5, 2020 19:34

ashleyjelks changed the title ~~[WIP] Collections Artwork Filter~~ [WIP] Collections Artwork Filter Container Feb 5, 2020

ashleyjelks force-pushed the collections-filter branch 2 times, most recently from 89fac70 to a8d5cb4 Compare February 6, 2020 23:01

ashleyjelks assigned sweir27 and unassigned ashleyjelks Feb 6, 2020

ashleyjelks requested review from brainbicycle and removed request for sweir27 February 6, 2020 23:08

ashleyjelks marked this pull request as ready for review February 6, 2020 23:08

ashleyjelks changed the title ~~[WIP] Collections Artwork Filter Container~~ Collections Artwork Filter Container Feb 6, 2020

ashleyjelks requested a review from pepopowitz February 6, 2020 23:15

ashleyjelks force-pushed the collections-filter branch from 0465149 to 900549f Compare February 6, 2020 23:43

pepopowitz reviewed Feb 7, 2020

View reviewed changes

ashleyjelks force-pushed the collections-filter branch 2 times, most recently from 55d0818 to 4e988f1 Compare February 10, 2020 17:00

sweir27 reviewed Feb 10, 2020

View reviewed changes

ashleyjelks force-pushed the collections-filter branch 2 times, most recently from 8702ca3 to 1df5e3f Compare February 11, 2020 17:57

ashleyjelks and others added 4 commits February 11, 2020 18:58

adds toggling filter button

37d81d8

Adds WIP Filter modal for Collections.

15ffdb9

Adds animation for slide up, and notes for next steps.

7fa584a

merge collection artwork filter PRs into single branch

3d28c10

ashleyjelks added 3 commits February 11, 2020 18:58

adds close modal button

00c6e25

adds feature flag to show/hide filter button

98cea5a

updates snapshots

bbea0cb

ashleyjelks force-pushed the collections-filter branch from 1df5e3f to bbea0cb Compare February 11, 2020 17:58

pepopowitz approved these changes Feb 11, 2020

View reviewed changes

sweir27 reviewed Feb 11, 2020

View reviewed changes

sweir27 merged commit ba9d3cb into master Feb 11, 2020

sweir27 deleted the collections-filter branch February 11, 2020 21:00

artsyit added the released label Feb 11, 2020

anandaroop reviewed Feb 12, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collections Artwork Filter Container #2073

Collections Artwork Filter Container #2073

ashleyjelks commented Feb 3, 2020 •

edited

Loading

pepopowitz left a comment

pepopowitz Feb 7, 2020

pepopowitz Feb 7, 2020

ashleyjelks Feb 10, 2020 •

edited

Loading

pepopowitz Feb 10, 2020

ashleyjelks Feb 10, 2020

sweir27 left a comment

sweir27 Feb 10, 2020

pepopowitz left a comment

sweir27 Feb 11, 2020

sweir27 commented Feb 11, 2020

artsyit commented Feb 11, 2020

anandaroop left a comment

anandaroop Feb 11, 2020

anandaroop Feb 11, 2020

anandaroop Feb 12, 2020

anandaroop Feb 12, 2020


		import { FilterModal } from "lib/Components/FilterModal"

		it("looks like expected", () => {

Collections Artwork Filter Container #2073

Collections Artwork Filter Container #2073

Conversation

ashleyjelks commented Feb 3, 2020 • edited Loading

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

ashleyjelks Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sweir27 left a 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

sweir27 commented Feb 11, 2020

artsyit commented Feb 11, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleyjelks commented Feb 3, 2020 •

edited

Loading

ashleyjelks Feb 10, 2020 •

edited

Loading