Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
1 change: 1 addition & 0 deletions src/app/AppRegistry.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ export const modules = defineModules({
MyProfile,
{
isRootViewForTabName: "profile",
fullBleed: true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was needed to avoid having a white status bar as we have inside the career highlights.
Example:
Screenshot 2023-05-05 at 16 50 37

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

},
[MyProfileHeaderMyCollectionAndSavedWorksScreenQuery, MyCollectionScreenQuery]
),
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { FC, useEffect, useRef } from "react"

interface AutomountedBottomSheetModalProps extends BottomSheetModalProps {
Expand All @@ -25,7 +25,7 @@ export const AutomountedBottomSheetModal: FC<AutomountedBottomSheetModalProps> =
ref={ref}
enablePanDownToClose
keyboardBlurBehavior="restore"
backdropComponent={ArtworkListsBottomSheetBackdrop}
backdropComponent={DefaultBottomSheetBackdrop}
{...rest}
/>
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import { useColor } from "@artsy/palette-mobile"
import { BottomSheetBackdropProps } from "@gorhom/bottom-sheet"
import { useMemo } from "react"
import { TouchableWithoutFeedback } from "react-native"
import Animated, { Extrapolate, interpolate, useAnimatedStyle } from "react-native-reanimated"

const MAX_OPACITY = 0.5
const MAX_OPACITY = 0.4
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was not following the design requirements - I fixed it


export const ArtworkListsBottomSheetBackdrop = ({
interface DefaultBottomSheetBackdrop extends BottomSheetBackdropProps {
onClose?: () => void
// set to "close" to close the bottom sheet when the backdrop is pressed
pressBehavior?: "close"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍

}

export const DefaultBottomSheetBackdrop: React.FC<DefaultBottomSheetBackdrop> = ({
animatedIndex,
onClose,
pressBehavior,
style,
}: BottomSheetBackdropProps) => {
}) => {
const color = useColor()

// animated variables
Expand All @@ -31,5 +40,15 @@ export const ArtworkListsBottomSheetBackdrop = ({
[style, containerAnimatedStyle]
)

return <Animated.View style={containerStyle} />
return (
<TouchableWithoutFeedback
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

onPress={() => {
if (pressBehavior === "close") {
onClose?.()
}
}}
>
<Animated.View style={containerStyle} />
</TouchableWithoutFeedback>
)
}
69 changes: 49 additions & 20 deletions src/app/Components/MenuItem.tsx
Original file line number Diff line number Diff line change
@@ -1,50 +1,75 @@
import { Spacer, ChevronIcon, Flex, useColor, Text, TextProps } from "@artsy/palette-mobile"
import { Touchable } from "@artsy/palette-mobile"
import {
ChevronIcon,
Flex,
Spacer,
SpacingUnit,
SpacingUnitsTheme,
Text,
TextProps,
Touchable,
useColor,
} from "@artsy/palette-mobile"
import { StyleProp, ViewStyle } from "react-native"
import { ResponsiveValue } from "styled-system"

export const MenuItem: React.FC<{
disabled?: boolean
allowDisabledVisualClue?: boolean // grays out with reduced opacity when disabled
title: React.ReactNode
value?: React.ReactNode
text?: string
isBeta?: boolean
onPress?: () => void
chevron?: React.ReactNode
description?: string
disabled?: boolean
ellipsizeMode?: TextProps["ellipsizeMode"]
style?: StyleProp<ViewStyle>
icon?: React.ReactNode
isBeta?: boolean
py?: ResponsiveValue<SpacingUnit, SpacingUnitsTheme>
onPress?: () => void
rightView?: React.ReactNode
style?: StyleProp<ViewStyle>
text?: string
title: React.ReactNode
value?: React.ReactNode
}> = ({
title,
text,
value,
isBeta,
onPress,
disabled = false,
allowDisabledVisualClue = false,
description,
disabled = false,
chevron = (
<ChevronIcon
direction="right"
fill={disabled && allowDisabledVisualClue ? "black30" : "black60"}
/>
),
ellipsizeMode,
style,
icon,
isBeta,
onPress,
py,
rightView,
style,
text,
title,
value,
}) => {
const color = useColor()

return (
<Touchable onPress={onPress} underlayColor="black5" disabled={disabled}>
<Flex
flexDirection="row"
alignItems="center"
py="7.5px"
py={py ?? "7.5px"}
px={2}
style={style}
opacity={disabled && allowDisabledVisualClue ? 0.5 : 1}
>
<Flex>
<Text variant="sm-display">{title}</Text>
{!!icon && <Flex flex={1}>{icon}</Flex>}
<Flex flex={7}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change broke the Account Settings screen 😬

image

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was intentional to define how much screen estate to cover

<Flex>
<Text variant="sm-display">{title}</Text>
{!!description && (
<Text variant="sm-display" color="black60">
{description}
</Text>
)}
</Flex>
{!!isBeta && (
<Flex px={0.5} mx={1} backgroundColor={color("black10")}>
<Text
Expand Down Expand Up @@ -85,7 +110,11 @@ export const MenuItem: React.FC<{

{rightView}

{!!(onPress && chevron) && <Flex ml={1}>{chevron}</Flex>}
{!!(onPress && chevron) && (
<Flex ml={1} justifyContent="center">
{chevron}
</Flex>
)}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</Flex>
</Flex>
</Touchable>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import BottomSheet from "@gorhom/bottom-sheet"
import { fireEvent } from "@testing-library/react-native"
import { MyCollectionBottomSheetModalAdd } from "app/Scenes/MyCollection/Components/MyCollectionBottomSheetModals/MyCollectionBottomSheetModalAdd"
import { MyCollectionTabsStoreProvider } from "app/Scenes/MyCollection/State/MyCollectionTabsStore"
import { renderWithWrappers } from "app/utils/tests/renderWithWrappers"

describe("MyCollectionBottomSheetModalAdd", () => {
const TestRenderer = () => {
return (
<MyCollectionTabsStoreProvider injections={{ view: "Add" }}>
<BottomSheet index={0} snapPoints={["50%"]}>
<MyCollectionBottomSheetModalAdd />
</BottomSheet>
</MyCollectionTabsStoreProvider>
)
}

describe("Add Artists", () => {
it("navigates the user to add artists screen", () => {
const { getByText } = renderWithWrappers(<TestRenderer />)

const addArtistsButton = getByText("Add Artists")

fireEvent(addArtistsButton, "onPress")

// TODO: Add this test later
})
})

describe("Add Artworks", () => {
it("navigates the user to add artwork screen", () => {
const { getByText } = renderWithWrappers(<TestRenderer />)

const addArworksButton = getByText("Add Artworks")

fireEvent(addArworksButton, "onPress")

// TODO: Add this test later
})
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { ArtworkIcon, Flex, Separator, Text, UserMultiIcon } from "@artsy/palette-mobile"
import { BottomSheetView } from "@gorhom/bottom-sheet"

import { MenuItem } from "app/Components/MenuItem"
import { MyCollectionTabsStore } from "app/Scenes/MyCollection/State/MyCollectionTabsStore"
import { Tab } from "app/Scenes/MyProfile/MyProfileHeaderMyCollectionAndSavedWorks"
import { navigate, popToRoot } from "app/system/navigation/navigate"

export const MyCollectionBottomSheetModalAdd: React.FC<{}> = () => {
const setView = MyCollectionTabsStore.useStoreActions((actions) => actions.setView)

return (
<BottomSheetView>
<Flex>
<Text textAlign="center" variant="sm" fontWeight="500" pt={2} pb={4}>
Add to My Collection
</Text>
<Separator />
</Flex>
<Flex>
<MenuItem
title="Add Artists"
description="List the artists in your collection."
onPress={() => {
console.log("Add Artists")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. I always forget these logs. 😆

}}
icon={<UserMultiIcon height={24} width={24} />}
py="40px"
/>
<Separator />
<MenuItem
title="Add Artworks"
description="Upload images and details of an artwork in your collection."
onPress={() => {
navigate("my-collection/artworks/new", {
passProps: {
mode: "add",
source: Tab.collection,
onSuccess: () => {
// hide the bottom sheet
setView(null)
popToRoot()
},
},
})
}}
icon={<ArtworkIcon height={24} width={24} />}
py="40px"
/>
</Flex>
</BottomSheetView>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import {
MyCollectionBottomSheetModalView,
MyCollectionBottomSheetModals,
} from "app/Scenes/MyCollection/Components/MyCollectionBottomSheetModals/MyCollectionBottomSheetModals"
import {
MyCollectionTabsStoreProvider,
myCollectionTabsStoreModel,
} from "app/Scenes/MyCollection/State/MyCollectionTabsStore"
import { renderWithWrappers } from "app/utils/tests/renderWithWrappers"

describe("MyCollectionBottomSheetModals", () => {
const TestRenderer: React.FC<{ view?: MyCollectionBottomSheetModalView }> = ({ view }) => (
<MyCollectionTabsStoreProvider
runtimeModel={{
...myCollectionTabsStoreModel,
view: view ?? null,
}}
>
<MyCollectionBottomSheetModals />
</MyCollectionTabsStoreProvider>
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})

it("renders Add to my collection when the Add view is specified", () => {
const { getByText } = renderWithWrappers(<TestRenderer view="Add" />)
expect(getByText("Add to My Collection")).toBeTruthy()
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import BottomSheet, { BottomSheetProps } from "@gorhom/bottom-sheet"
import { DefaultBottomSheetBackdrop } from "app/Components/BottomSheet/DefaultBottomSheetBackdrop"
import { MyCollectionBottomSheetModalAdd } from "app/Scenes/MyCollection/Components/MyCollectionBottomSheetModals/MyCollectionBottomSheetModalAdd"
import { MyCollectionTabsStore } from "app/Scenes/MyCollection/State/MyCollectionTabsStore"
import { useCallback, useMemo, useRef } from "react"

export type MyCollectionBottomSheetModalView = "Add" | null

export const MyCollectionBottomSheetModals: React.FC<{}> = () => {
const bottomSheetRef = useRef<BottomSheet>(null)

const setView = MyCollectionTabsStore.useStoreActions((actions) => actions.setView)
const view = MyCollectionTabsStore.useStoreState((state) => state.view)

const snapPoints = useMemo(() => ["50%"], [])

const handleSheetChanges = useCallback((index: number) => {
if (index === -1) {
setView(null)
}
}, [])

const hideBottomSheet = () => {
if (view) {
bottomSheetRef.current?.close()
setView(null)
}
}

const renderBackdrop: BottomSheetProps["backdropComponent"] = (props) => {
return <DefaultBottomSheetBackdrop pressBehavior="close" onClose={hideBottomSheet} {...props} />
}

return (
<>
<BottomSheet
ref={bottomSheetRef}
index={0}
onChange={handleSheetChanges}
snapPoints={snapPoints}
enablePanDownToClose
backdropComponent={renderBackdrop}
handleIndicatorStyle={{ backgroundColor: "black", width: 40, height: 4, borderRadius: 2 }}
>
{view === "Add" && <MyCollectionBottomSheetModalAdd />}
</BottomSheet>
</>
)
}
Loading