Es/chore replace toast with modal #310
Conversation
| }, [prevRouteNameInStack, CustomHeaderLeft, CustomHeaderLeftClose]); | ||
| }, [prevRouteNameInStack, openSheet, navigation]); |
There was a problem hiding this comment.
eslint told me to take out CustomHeaderLeft and CustomHeaderLeftClose....?
There was a problem hiding this comment.
that's correct - they are stable functions under the hood
There was a problem hiding this comment.
Think there's a good opportunity here to clean up the way that our custom bottom sheet modal is used. It's never been clearly documented, but the original design of the component when i implemented it long ago is that it the BottomSheetModal should (almost) always be co-located with the usage of the useBottomSheetModal() hook. Instead, i'm noticing that custom bottom sheet modal components are including the BottomSheetModal and unnecessarily accepting and passing props that come from the hook in the consuming component. what should be happening is that the bottom sheet contents can/should be componentized when appropriate, and then used as a child of the BottomSheetModal that's being used in the consuming component.
I added a patch that lives on a different branch to demonstrate the ideal usage i'm describing. Diff between this PR and that patch can be found here. the summary of changes in that patch are:
- Create a directory called
contentVariantsthat lives insharedComponents/BottomSheet/. This serves as a collection of sheet content implementations that are geared towards specific usages based on common patterns. - Implement
contentVariants/Destructive.tsx, which is a "foundational" sheet content component that is only opinionated about the icons being used to match our design patterns (in this case, the error icon at the top and the delete icon within the destructive button) - Implement
contentVariants/ObservationDiscard.tsx, which usesDestructivebut is opinionated about the content (e.g. title, description, text in buttons). It accepts callback props that are more semantically meaningful but not functionally different thanDestructive(just passed along directly). - Update
ObservationEdit/index.tsxandPresetChooser.tsxto useBottomSheetModalandObservationDiscard - Update
SaveTrackScreen.tsxto useBottomSheetModalandDestructive - Remove
DiscardModalin its entirety. In its current form (aka prior to this PR), it's a non-useful wrapper. Should've caught that in previous PRs
| }, [prevRouteNameInStack, CustomHeaderLeft, CustomHeaderLeftClose]); | ||
| }, [prevRouteNameInStack, openSheet, navigation]); |
There was a problem hiding this comment.
that's correct - they are stable functions under the hood
| // const handleDetailsPress = React.useCallback(() => { | ||
| // navigation.navigate('ObservationDetails', {question: 1}); | ||
| // }, [navigation]); |
There was a problem hiding this comment.
remove?
| // const handleDetailsPress = React.useCallback(() => { | |
| // navigation.navigate('ObservationDetails', {question: 1}); | |
| // }, [navigation]); |
| @@ -61,17 +27,20 @@ interface SharedBackButtonProps { | |||
|
|
|||
| type CustomHeaderLeftCloseProps = { | |||
| observationId?: string; | |||
| openDiscardModal: () => void; | |||
There was a problem hiding this comment.
seeing this being added is making me notice the scope creep of this component, but i think addressing that is outside the scope of this PR. can create an issue to provide more details of what a refactor could entail
| <DiscardModal | ||
| bottomSheetRef={sheetRef} | ||
| isOpen={isOpen} | ||
| closeSheet={closeSheet} | ||
| title={deleteObservationMessages.discardObservation} | ||
| description={deleteObservationMessages.discardObservationDescription} | ||
| discardButtonText={deleteObservationMessages.discardObservationButton} | ||
| handleDiscard={handleDiscard} | ||
| /> |
There was a problem hiding this comment.
won't this cause issues with the error modal that's also used in this component?
|
Closing this to reimplement in a cleaner way. |
Replaces android native toast with a bottom sheet.