-
Notifications
You must be signed in to change notification settings - Fork 573
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
Changes from all commits
9e1354b
4012407
1dddc14
15a38ca
0cccd40
8080bae
c276ddf
f7c445c
ba012c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -25,7 +25,7 @@ export const AutomountedBottomSheetModal: FC<AutomountedBottomSheetModalProps> = | |
ref={ref} | ||
enablePanDownToClose | ||
keyboardBlurBehavior="restore" | ||
backdropComponent={ArtworkListsBottomSheetBackdrop} | ||
backdropComponent={DefaultBottomSheetBackdrop} | ||
{...rest} | ||
/> | ||
) | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -31,5 +40,15 @@ export const ArtworkListsBottomSheetBackdrop = ({ | |
[style, containerAnimatedStyle] | ||
) | ||
|
||
return <Animated.View style={containerStyle} /> | ||
return ( | ||
<TouchableWithoutFeedback | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 RPReplay_Final1683533203.1.mp4 |
||
onPress={() => { | ||
if (pressBehavior === "close") { | ||
onClose?.() | ||
} | ||
}} | ||
> | ||
<Animated.View style={containerStyle} /> | ||
</TouchableWithoutFeedback> | ||
) | ||
} |
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}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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> | ||
)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
</> | ||
) | ||
} |
There was a problem hiding this comment.
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:
Fwiw, I added a top padding later inside the screen to make sure we don't cover the sticky tabs header