feat(CX-3636): create add to my collection bottom sheet #8631

MounirDhahri · 2023-05-05T07:52:17Z

This PR resolves CX-3636

Description

This PR adds "Add to my collection" bottom sheet inside my collection. It also lays the foundation of the future work around the bottom sheet modal.

For easier review, I recommend hiding spaces like below ⬇️ .

Screen Recording

Screen.Recording.2023-05-05.at.16.38.31.mov

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

Create Add to my collection bottom sheet - mounir

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.

MounirDhahri · 2023-05-05T14:52:17Z

src/app/AppRegistry.tsx

@@ -454,6 +454,7 @@ export const modules = defineModules({
    MyProfile,
    {
      isRootViewForTabName: "profile",
+      fullBleed: true,


this was needed to avoid having a white status bar as we have inside the career highlights.
Example:

Fwiw, I added a top padding later inside the screen to make sure we don't cover the sticky tabs header

MounirDhahri · 2023-05-05T14:52:53Z

src/app/Components/ArtworkLists/components/AutomountedBottomSheetModal.tsx

@@ -1,5 +1,5 @@
 import { BottomSheetModal, BottomSheetModalProps } from "@gorhom/bottom-sheet"
-import { ArtworkListsBottomSheetBackdrop } from "app/Components/ArtworkLists/components/ArtworkListsBottomSheetBackdrop"
+import { DefaultBottomSheetBackdrop } from "app/Components/BottomSheet/DefaultBottomSheetBackdrop"


Since we no longer use this only inside artwork list, I extracted the component outside

MounirDhahri · 2023-05-05T14:53:19Z

src/app/Components/BottomSheet/DefaultBottomSheetBackdrop.tsx

 import Animated, { Extrapolate, interpolate, useAnimatedStyle } from "react-native-reanimated"

-const MAX_OPACITY = 0.5
+const MAX_OPACITY = 0.4


this was not following the design requirements - I fixed it

MounirDhahri · 2023-05-05T14:54:30Z

src/app/Components/BottomSheet/DefaultBottomSheetBackdrop.tsx

+interface DefaultBottomSheetBackdrop extends BottomSheetBackdropProps {
+  onClose?: () => void
+  // set to "close" to close the bottom sheet when the backdrop is pressed
+  pressBehavior?: "close"


using this behavior here to match the default BottomSheetBackdrop from the bottom sheet package. probably an overkill but I see no harm and it doesn't affect the code readability

Good idea 👍

MounirDhahri · 2023-05-05T14:55:04Z

src/app/Components/BottomSheet/DefaultBottomSheetBackdrop.tsx

@@ -30,5 +39,15 @@ export const ArtworkListsBottomSheetBackdrop = ({
    [style, containerAnimatedStyle]
  )

-  return <Animated.View style={containerStyle} />
+  return (
+    <TouchableWithoutFeedback


using a touchable without feedback here because it's a common pattern for drawers

I wonder if we should use Palette's Touchable component with noFeedback prop set to true here 🤔

I actually used that initially but reverted it later. Main reason behind my thinking was that we currently use Touchable for buttons and the backdrop in this case is a view that accepts touches. I wanted therefore to not use any abstraction around it to avoid changing its behaviour in case we extend the Touchable component later.
This seems to be the pattern used in Instagram as well

RPReplay_Final1683533203.1.mp4

MounirDhahri · 2023-05-05T14:55:48Z

src/app/Components/MenuItem.tsx

+            <Flex ml={1} justifyContent="center">
+              {chevron}
+            </Flex>
+          )}


I tested all the changes above in the surface areas that I know we are using the menu item and nothing seems to have break

MounirDhahri · 2023-05-05T14:57:08Z

...yCollection/Components/MyCollectionBottomSheetModals/MyCollectionBottomSheetModals.tests.tsx

+
+  it("renders nothing", () => {
+    const { getByText } = renderWithWrappers(<TestRenderer />)
+    expect(() => getByText("Add to My Collection")).toThrow()


I think this has potential to be improved but I would rather do that later when we add the remaining code the bottom sheet modal

MounirDhahri · 2023-05-05T14:57:50Z

src/app/Scenes/MyCollection/State/MyCollectionTabsStore.tsx

+
+export const MyCollectionTabsStore = createContextStore((injections) => ({
+  ...myCollectionTabsStoreModel,
+  ...injections,


I needed injections here to make testing the component easier

MounirDhahri · 2023-05-05T14:59:32Z

src/app/Scenes/MyCollection/State/MyCollectionTabsStore.tsx

  selectedTab: null,
+  view: null,


because we trigger showing the modal from the stickytabs + we need the bottomsheet to be next to the main page (my collection), I am using a store here to display the right content.
As we build more on top of this, we might need to switch to inject jsx into this context store as we do for the sticky tab page but I will leave that to when we get there

olerichter00

Great work 🌟

olerichter00 · 2023-05-08T07:43:20Z

src/app/Components/BottomSheet/DefaultBottomSheetBackdrop.tsx

+interface DefaultBottomSheetBackdrop extends BottomSheetBackdropProps {
+  onClose?: () => void
+  // set to "close" to close the bottom sheet when the backdrop is pressed
+  pressBehavior?: "close"


Good idea 👍

olerichter00 · 2023-05-08T07:44:54Z

src/app/Components/BottomSheet/DefaultBottomSheetBackdrop.tsx

@@ -30,5 +39,15 @@ export const ArtworkListsBottomSheetBackdrop = ({
    [style, containerAnimatedStyle]
  )

-  return <Animated.View style={containerStyle} />
+  return (
+    <TouchableWithoutFeedback


I wonder if we should use Palette's Touchable component with noFeedback prop set to true here 🤔

olerichter00 · 2023-05-08T07:46:23Z

src/app/Components/MenuItem.tsx

-          <Text variant="sm-display">{title}</Text>
+        {!!icon && <Flex flex={1}>{icon}</Flex>}
+        <Flex flex={7}>
+          {/* Description */}


Probably not needed and it's followed by the title...

Suggested change

{/* Description */}

I always find it easier to use blocks of comments to separate large pieces of logic inside big components. I feel it makes it easier for the next dev to know what is what without reading the code (which is why I added these comments). I will remove it here since we would like to get this out before sprint starting, can discuss it further tomorrow.

That makes sense 👍 I was just confused because the comment {/* Description */} is followed by the implementation of the title and not the description....

olerichter00 · 2023-05-08T07:46:33Z

src/app/Components/MenuItem.tsx

@@ -59,6 +85,7 @@ export const MenuItem: React.FC<{

        <Spacer x={2} />

+        {/* Value */}


olerichter00 · 2023-05-08T07:48:22Z

...ollection/Components/MyCollectionBottomSheetModals/MyCollectionBottomSheetModalAdd.tests.tsx

+      // replace with the right mock
+      // expect(console.log).toHaveBeenCalledWith("Add Artists")


suggestion: What do you think about adding TODO: here? This would give some editors the change to highlight the comment to make it more visible (+ Jira integration etc.).

good idea - will do it now. I have the same integration as well, it's useful to take advtange of it

olerichter00 · 2023-05-08T07:48:41Z

...ollection/Components/MyCollectionBottomSheetModals/MyCollectionBottomSheetModalAdd.tests.tsx

+
+      fireEvent(addArworksButton, "onPress")
+
+      // Will add this test later


sure, this is a todo for the next person building the add artworks flow 😄

olerichter00 · 2023-05-08T07:49:15Z

...es/MyCollection/Components/MyCollectionBottomSheetModals/MyCollectionBottomSheetModalAdd.tsx

+          title="Add Artists"
+          description="List the artists in your collection."
+          onPress={() => {
+            console.log("Add Artists")


I have this intentionally here Ole to make it easier to find it for future work by looking in the project. @dariakoko is already working on it. None of this code is available in prod of course.
I sometimes use TODO:// or simple console log it to make it easier to look for the button. I can gladly change this of course

Ah, I see, this will be implemented in the next step anyway 💡

I like the idea of adding an additional TODO: to make sure these console.logs will be removed at some point. Probably it's only me who always forgets to remove them 😄

Same. I always forget these logs. 😆

lordkiz

🚢 🚀

MrSltun · 2023-05-11T14:13:15Z

src/app/Components/MenuItem.tsx

-        <Flex>
-          <Text variant="sm-display">{title}</Text>
+        {!!icon && <Flex flex={1}>{icon}</Flex>}
+        <Flex flex={7}>


I believe this change broke the Account Settings screen 😬

Was the flex={7} intentional? or would it be okay to remove it?

it was intentional to define how much screen estate to cover

artsy-peril bot added the Jira Synced label May 5, 2023

MounirDhahri force-pushed the feat/CX-3636/create-add-to-my-collection-bottom-sheet branch 2 times, most recently from ab4074a to 558bac0 Compare May 5, 2023 14:41

MounirDhahri assigned MounirDhahri and dariakoko and unassigned dariakoko May 5, 2023

MounirDhahri requested review from lordkiz, dariakoko, olerichter00 and MrSltun May 5, 2023 14:43

MounirDhahri commented May 5, 2023

View reviewed changes

MounirDhahri changed the title ~~feat(CX-3636): create add to my collection bottom sheet [WIP]~~ feat(CX-3636): create add to my collection bottom sheet May 5, 2023

MounirDhahri force-pushed the feat/CX-3636/create-add-to-my-collection-bottom-sheet branch from 58bf644 to f125c7c Compare May 5, 2023 15:16

MounirDhahri marked this pull request as ready for review May 5, 2023 15:16

olerichter00 previously approved these changes May 8, 2023

View reviewed changes

MounirDhahri dismissed olerichter00’s stale review via 5357198 May 8, 2023 08:23

MounirDhahri added 4 commits May 8, 2023 10:27

refactor: fix linter

9e1354b

feat: define bottom sheet modal

4012407

refactor: fix context warnings

1dddc14

feat: add to my collection bottom sheet modal

15a38ca

MounirDhahri added 5 commits May 8, 2023 10:27

refactor: update bottom sheet modal

0cccd40

chore: add more tests

8080bae

chore: hide bottom sheet after adding an artwork

c276ddf

chore: fix broken test

f7c445c

chore: address review comments

ba012c9

MounirDhahri force-pushed the feat/CX-3636/create-add-to-my-collection-bottom-sheet branch from 5357198 to ba012c9 Compare May 8, 2023 08:28

MounirDhahri added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label May 8, 2023

artsy-peril bot merged commit 7231d31 into main May 8, 2023

artsy-peril bot deleted the feat/CX-3636/create-add-to-my-collection-bottom-sheet branch May 8, 2023 08:38

lordkiz reviewed May 8, 2023

View reviewed changes

MrSltun reviewed May 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(CX-3636): create add to my collection bottom sheet #8631

feat(CX-3636): create add to my collection bottom sheet #8631

MounirDhahri commented May 5, 2023 •

edited

Loading

MounirDhahri May 5, 2023

MounirDhahri May 5, 2023

MounirDhahri May 5, 2023

MounirDhahri May 5, 2023

olerichter00 May 8, 2023

MounirDhahri May 5, 2023

olerichter00 May 8, 2023

MounirDhahri May 8, 2023

MounirDhahri May 5, 2023

MounirDhahri May 5, 2023

MounirDhahri May 5, 2023

MounirDhahri May 5, 2023

olerichter00 left a comment

olerichter00 May 8, 2023

olerichter00 May 8, 2023

olerichter00 May 8, 2023

MounirDhahri May 8, 2023

olerichter00 May 8, 2023

olerichter00 May 8, 2023

olerichter00 May 8, 2023

MounirDhahri May 8, 2023

olerichter00 May 8, 2023

MounirDhahri May 8, 2023

olerichter00 May 8, 2023

MounirDhahri May 8, 2023

olerichter00 May 8, 2023

lordkiz May 8, 2023

lordkiz left a comment •

edited

Loading

MrSltun May 11, 2023

MounirDhahri May 11, 2023

		@@ -59,6 +85,7 @@ export const MenuItem: React.FC<{

		<Spacer x={2} />

		{/* Value */}

		// replace with the right mock
		// expect(console.log).toHaveBeenCalledWith("Add Artists")


		fireEvent(addArworksButton, "onPress")

		// Will add this test later

feat(CX-3636): create add to my collection bottom sheet #8631

feat(CX-3636): create add to my collection bottom sheet #8631

Conversation

MounirDhahri commented May 5, 2023 • 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

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

olerichter00 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

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

lordkiz 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

MounirDhahri commented May 5, 2023 •

edited

Loading

lordkiz left a comment •

edited

Loading