Implement Initial Collections Artwork Filter Navigation Stack #2096

ashleyjelks · 2020-02-20T13:26:20Z

#skip_new_tests

sweir27

Few comments (mostly on tests) but this is looking great!!

sweir27 · 2020-02-21T13:38:24Z

src/lib/Components/ArtworkFilterOptions/__tests__/SortOptions-tests.tsx

+        <SortOptions navigator={mockNavigator as any} />
+      </Theme>
+    )
+    expect(SortOptionsScreen).toMatchSnapshot()


I tend to find snapshots not super helpful. Doesn't have to be for this PR, but instead could we test a meaningful aspect of the component? (Such as that the correct number of OptionListItems show up)?

sweir27 · 2020-02-21T13:38:42Z

src/lib/Components/FilterModal.tsx

+                    navigationBarHidden={true}
+                    initialRoute={{
+                      component: FilterOptions,
+                      title: "",


I don't think we need this.

sweir27 · 2020-02-21T13:41:23Z

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

+        <FilterOptions navigator={mockNavigator as any} closeModal={jest.fn()} />
+      </Theme>
+    )
+    expect(FilterOptionsScreen).toMatchSnapshot()


Here too, I think this test would be more meaningful with more explicit assertions! (But not a blocker)

sweir27 · 2020-02-21T13:42:12Z

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

+        <FilterOptions closeModal={jest.fn()} navigator={mockNavigator as any} />
+      </Theme>
+    )
+    const filterListItem = filterScreen.root.findByProps({ "data-test-filter-item": "item" })


It seems a little odd to pass props in order to find the item. Can we select for this based on the component's text or class name? (this applies to the other places as well!)

sweir27 · 2020-02-21T13:42:40Z

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

-    .create(
+    const getNextScreenTitle = component => component.root.findByType(Sans).props.children
+
+    expect(getNextScreenTitle(filterScreen)).toEqual("Sort")


sweir27 · 2020-02-21T13:44:01Z

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

+    expect(getPreviousScreenTitle(filterScreen)).toEqual("Filter")
+  })
+
+  it("allows users to eit filter modal when selecting close icon", () => {


#superminor eit -> exit

ds300

Looking really good for a start!

ds300 · 2020-02-21T13:50:14Z

src/lib/Components/FilterModal.tsx

+    const { filterOptions } = this.state
+
+    return (
+      <Flex flexGrow={1} data-test-filter-title="item">


this data-test-[element-id]="item" pattern confused me a little. React Native elements have a prop called testID that people use for the same thing but it uses the form testID="[element-id]", so in this case instead of data-test-filter-title="item" it would be testID="filter-title"

Although, having said that, our palette components use the web typings (😬) so typescript would not allow testID, but you could do data-test-id="filter-title" to keep the same naming pattern which would probably be less surprising to other folks reading this code later.

edit: sarah's suggestion is also good 👍 😅

Yes, I was trying to avoid creating aliased styled-components simply for test purposes as there are multiple instances of the same component type and I needed a way to grab a specific instance. But it seems like there are trade-offs either way.

ds300 · 2020-02-21T14:00:35Z

src/lib/Components/ArtworkFilterOptions/SortOptions.tsx

+  renderSortOption = ({ item }) => {
+    return (
+      <OptionListItem>
+        <Flex p={2} flexDirection="row" justifyContent="space-between" flexGrow={1}>
+          <Serif size="3">{item}</Serif>
+        </Flex>
+      </OptionListItem>
+    )
+  }


would be good to have this typed, or personally I'd just move the jsx inline into the renderItem prop so you get the typing for free

src/lib/Components/ArtworkFilterOptions/SortOptions.tsx

ds300 · 2020-02-21T14:32:13Z

src/lib/Components/ArtworkFilterOptions/SortOptions.tsx

+      <Flex flexGrow={1}>
+        <SortHeader>
+          <Flex alignItems="flex-end" mt={0.5} mb={2}>
+            <Box ml={2} mt={2} onTouchStart={() => this.handleBackNavigation()} data-test-sort-back-nav="item">


normally on mobile we wouldn't handle presses using onTouchStart because it fires as soon as the user's finger hits the area, but they might not have intended to press the button. It's best to use something like <TouchableWithOpacity onPress={...}> which handles this situation.

ds300 · 2020-02-21T14:34:47Z

src/lib/Components/ArtworkFilterOptions/SortOptions.tsx

+export const SortHeader = styled(Flex)`
+  flex-direction: row;
+  justify-content: space-between;
+  padding-right: 20px;


we should probably use the space helper from palette here

Suggested change

padding-right: 20px;

padding-right: ${space(2)}px;

ds300 · 2020-02-21T15:26:08Z

src/lib/Components/ArtworkFilterOptions/SortOptions.tsx

+      <Flex flexGrow={1}>
+        <SortHeader>
+          <Flex alignItems="flex-end" mt={0.5} mb={2}>
+            <ArrowLeftIconContainer onTouchStart={() => this.handleBackNavigation()}>


again, would be good to use TouchableOpacity here

ds300 · 2020-02-21T15:27:21Z

src/lib/Components/FilterModal.tsx

-                    onPress={() => {
-                      this.closeModal()
+              <ModalBackgroundView>
+                <Flex onTouchStart={this.props.closeModal} style={{ flexGrow: 1 }} />


would be good to use TouchableWithoutFeedback with onPress here

ds300 · 2020-02-21T15:28:55Z

src/lib/Components/FilterModal.tsx

+      <OptionListItem>
+        <Flex p={2} flexDirection="row" justifyContent="space-between" flexGrow={1}>
+          <Serif size="3">{type}</Serif>
+          <TouchableOptionListItemRow onTouchStart={() => onTap()}>


again, would be good to use TouchableOpacity with onPress

ds300 · 2020-02-21T15:45:51Z

src/lib/Components/FilterModal.tsx

+    const filterOptions = []
+
+    filterOptions.push({
+      type: "Sort by",
+      onTap: this.handleNavigationToSortScreen,
+    })
+
+    this.setState({
+      filterOptions,
+    })


Looks like this state value never changes so you can just inline it in the render method.

@ashleyjelks this is another opportunity for cleanup!

ds300 · 2020-02-21T15:48:00Z

src/lib/Components/FilterModal.tsx

+      <Flex flexGrow={1}>
+        <Flex flexDirection="row" justifyContent="space-between">
+          <Flex alignItems="flex-end" mt={0.5} mb={2}>
+            <CloseIconContainer onTouchStart={() => this.props.closeModal()}>


💡 TouchableOpcaity + onPress

sweir27 · 2020-02-21T19:37:20Z

Cool! @ashleyjelks I'm going to merge this in 😄 but it's worth revisiting the few remaining items from @ds300's review in one of your subsequent PRs!

artsyit · 2020-02-21T19:58:16Z

🚀 PR was released in v1.21.52 🚀

ashleyjelks added the DO NOT MERGE label Feb 20, 2020

ashleyjelks self-assigned this Feb 20, 2020

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

ashleyjelks force-pushed the filter-nav-stack branch from a2941d2 to 8f1e386 Compare February 20, 2020 13:28

ashleyjelks changed the title ~~Filter nav stack WIP Do Not Merge~~ Implement Initial Collections Artwork Filter Navigation Stack Feb 20, 2020

ashleyjelks assigned sweir27 and unassigned ashleyjelks Feb 20, 2020

ashleyjelks requested review from ds300 and xtina-starr February 20, 2020 18:02

ashleyjelks marked this pull request as ready for review February 20, 2020 18:02

ashleyjelks removed the DO NOT MERGE label Feb 21, 2020

sweir27 reviewed Feb 21, 2020

View reviewed changes

ashleyjelks force-pushed the filter-nav-stack branch from 0ec266f to b42336f Compare February 21, 2020 14:53

ashleyjelks added 10 commits February 21, 2020 16:17

adds sort options to modal

c43042c

WIP Commit--get nav screens working

bb9095a

wip commit--get sort screen rendering

ea2207f

wip commit--cleans up styling on flat list

f4dd7a4

more ui fixes WIP

b51e668

adds tests

6e0b473

removes outdated files

b15e568

re-org file structures

53781f9

adds snapshot for sort options screen

8eb2baa

updates filter modal tests

b958eeb

ashleyjelks force-pushed the filter-nav-stack branch from b42336f to b958eeb Compare February 21, 2020 15:18

ds300 reviewed Feb 21, 2020

View reviewed changes

ashleyjelks added 2 commits February 21, 2020 18:16

removes deprecated storybook components

d80ea03

fadfdsa

570dd84

sweir27 merged commit bab77fb into master Feb 21, 2020

sweir27 deleted the filter-nav-stack branch February 21, 2020 19:37

artsyit added the released label Feb 21, 2020

ashleyjelks mentioned this pull request Feb 24, 2020

Fixes bug introduced via Eigen/Emission rebase artsy/eigen#3032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Initial Collections Artwork Filter Navigation Stack #2096

Implement Initial Collections Artwork Filter Navigation Stack #2096

ashleyjelks commented Feb 20, 2020 •

edited

Loading

sweir27 left a comment

sweir27 Feb 21, 2020

sweir27 Feb 21, 2020

sweir27 Feb 21, 2020

sweir27 Feb 21, 2020

sweir27 Feb 21, 2020

sweir27 Feb 21, 2020

ds300 left a comment

ds300 Feb 21, 2020 •

edited

Loading

ashleyjelks Feb 21, 2020

ds300 Feb 21, 2020

ds300 Feb 21, 2020

ds300 Feb 21, 2020

ds300 Feb 21, 2020

ds300 Feb 21, 2020

ds300 Feb 21, 2020

ds300 Feb 21, 2020

sweir27 Feb 21, 2020

ds300 Feb 21, 2020

sweir27 commented Feb 21, 2020

artsyit commented Feb 21, 2020

Implement Initial Collections Artwork Filter Navigation Stack #2096

Implement Initial Collections Artwork Filter Navigation Stack #2096

Conversation

ashleyjelks commented Feb 20, 2020 • edited Loading

sweir27 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

ds300 left a comment

Choose a reason for hiding this comment

ds300 Feb 21, 2020 • 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

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

sweir27 commented Feb 21, 2020

artsyit commented Feb 21, 2020

ashleyjelks commented Feb 20, 2020 •

edited

Loading

ds300 Feb 21, 2020 •

edited

Loading