-
Notifications
You must be signed in to change notification settings - Fork 24
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
(PC-33485) feat(Modals): resolve modal conflicts #7427
base: master
Are you sure you want to change the base?
Changes from all commits
68c5f31
0e1371e
1b39457
cd0b049
9bc0335
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 |
---|---|---|
|
@@ -9,10 +9,59 @@ import { ReactionChoiceModal } from 'features/reactions/components/ReactionChoic | |
import { ReactionChoiceModalBodyEnum, ReactionFromEnum } from 'features/reactions/enum' | ||
import { OfferImageBasicProps } from 'features/reactions/types' | ||
import { formatToSlashedFrenchDate } from 'libs/dates' | ||
import { useFeatureFlag } from 'libs/firebase/firestore/featureFlags/useFeatureFlag' | ||
import { RemoteStoreFeatureFlags } from 'libs/firebase/firestore/types' | ||
import { useRemoteConfigContext } from 'libs/firebase/remoteConfig/RemoteConfigProvider' | ||
import { reactionModal } from 'libs/modals/modals' | ||
import { useCategoryIdMapping, useSubcategoriesMapping } from 'libs/subcategories' | ||
import { useModal } from 'ui/components/modals/useModal' | ||
|
||
export const useIncomingReactionModal = () => { | ||
const isReactionFeatureActive = useFeatureFlag(RemoteStoreFeatureFlags.WIP_REACTION_FEATURE) | ||
const { reactionCategories } = useRemoteConfigContext() | ||
const { isCookiesListUpToDate, cookiesLastUpdate } = useIsCookiesListUpToDate() | ||
const isCookieConsentChecked = cookiesLastUpdate && isCookiesListUpToDate | ||
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. pourquoi les réactions sont couplés aux cookies ? |
||
const { data: bookings } = useBookings() | ||
const subcategoriesMapping = useSubcategoriesMapping() | ||
const mapping = useCategoryIdMapping() | ||
|
||
const bookingsWithoutReaction = | ||
bookings?.ended_bookings?.filter((booking) => | ||
filterBookingsWithoutReaction(booking, subcategoriesMapping, reactionCategories) | ||
) ?? [] | ||
|
||
const offerImages: OfferImageBasicProps[] = bookingsWithoutReaction.map((current) => { | ||
return { | ||
imageUrl: current.stock.offer.image?.url ?? '', | ||
categoryId: mapping[current.stock.offer.subcategoryId] ?? null, | ||
} | ||
}) | ||
|
||
const firstBooking = bookingsWithoutReaction[0] | ||
const reactionChoiceModalBodyType = | ||
bookingsWithoutReaction.length === 1 | ||
? ReactionChoiceModalBodyEnum.VALIDATION | ||
: ReactionChoiceModalBodyEnum.REDIRECTION | ||
Comment on lines
+28
to
+44
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. si cette partie était faite dans 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. ça pourrait éviter quelques calculs dans le cas où une autre modal s'affiche avant celle ci |
||
|
||
const getModal = () => { | ||
if (!firstBooking || !isCookieConsentChecked || !isReactionFeatureActive) return | ||
|
||
const { stock, dateUsed } = firstBooking | ||
const { offer } = stock | ||
|
||
return reactionModal({ | ||
offer, | ||
dateUsed: dateUsed ? `le ${formatToSlashedFrenchDate(dateUsed)}` : '', | ||
defaultReaction: null, | ||
from: ReactionFromEnum.HOME, | ||
bodyType: reactionChoiceModalBodyType, | ||
offerImages, | ||
}) | ||
} | ||
|
||
return getModal | ||
} | ||
|
||
export const IncomingReactionModalContainer = () => { | ||
const { reactionCategories } = useRemoteConfigContext() | ||
const { isCookiesListUpToDate, cookiesLastUpdate } = useIsCookiesListUpToDate() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,21 +7,21 @@ import { useAuthContext } from 'features/auth/context/AuthContext' | |
import { useBookings } from 'features/bookings/api' | ||
import { useHomepageData } from 'features/home/api/useHomepageData' | ||
import { HomeHeader } from 'features/home/components/headers/HomeHeader' | ||
import { IncomingReactionModalContainer } from 'features/home/components/IncomingReactionModalContainer/IncomingReactionModalContainer' | ||
import { useIncomingReactionModal } from 'features/home/components/IncomingReactionModalContainer/IncomingReactionModalContainer' | ||
import { HomeBanner } from 'features/home/components/modules/banners/HomeBanner' | ||
import { PERFORMANCE_HOME_CREATION, PERFORMANCE_HOME_LOADING } from 'features/home/constants' | ||
import { GenericHome } from 'features/home/pages/GenericHome' | ||
import { UseRouteType } from 'features/navigation/RootNavigator/types' | ||
import { AchievementSuccessModal } from 'features/profile/components/Modals/AchievementSuccessModal' | ||
import { useShouldShowAchievementSuccessModal } from 'features/profile/components/Modals/useShouldShowAchievementSuccessModal' | ||
import { OnboardingSubscriptionModal } from 'features/subscription/components/modals/OnboardingSubscriptionModal' | ||
import { useOnboardingSubscriptionModal } from 'features/subscription/helpers/useOnboardingSubscriptionModal' | ||
import { analytics } from 'libs/analytics' | ||
import { useFeatureFlag } from 'libs/firebase/firestore/featureFlags/useFeatureFlag' | ||
import { RemoteStoreFeatureFlags } from 'libs/firebase/firestore/types' | ||
import { useFunctionOnce } from 'libs/hooks' | ||
import { useLocation } from 'libs/location' | ||
import { LocationMode } from 'libs/location/types' | ||
import { createRelativeModals } from 'libs/modals/modal.creator' | ||
import { useModalActions } from 'libs/modals/modal.store' | ||
import { achievementsModal, reactionModal } from 'libs/modals/modals' | ||
import { getAppVersion } from 'libs/packageJson' | ||
import { BatchProfile } from 'libs/react-native-batch' | ||
import { startTransaction } from 'shared/performance/transactions' | ||
|
@@ -34,11 +34,15 @@ const Header = () => ( | |
</ListHeaderContainer> | ||
) | ||
|
||
let hasShowSessionModal = false | ||
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. avoir une variable globale mutable me fait peur sur les effets de bord que ça pourrait avoir |
||
|
||
export const Home: FunctionComponent = () => { | ||
const { openModal } = useModalActions() | ||
const startPerfHomeLoadingOnce = useFunctionOnce(() => startTransaction(PERFORMANCE_HOME_LOADING)) | ||
const startPerfHomeCreationOnce = useFunctionOnce(() => | ||
startTransaction(PERFORMANCE_HOME_CREATION) | ||
) | ||
|
||
startPerfHomeCreationOnce() | ||
startPerfHomeLoadingOnce() | ||
|
||
|
@@ -53,26 +57,33 @@ export const Home: FunctionComponent = () => { | |
showModal: showOnboardingSubscriptionModal, | ||
hideModal: hideOnboardingSubscriptionModal, | ||
} = useModal(false) | ||
|
||
useOnboardingSubscriptionModal({ | ||
isLoggedIn, | ||
userStatus: user?.status?.statusType, | ||
showOnboardingSubscriptionModal, | ||
}) | ||
const { shouldShowAchievementSuccessModal, achievementsToShow } = | ||
useShouldShowAchievementSuccessModal() | ||
const { | ||
visible: visibleAchievementModal, | ||
showModal: showAchievementModal, | ||
hideModal: hideAchievementModal, | ||
} = useModal(false) | ||
|
||
const checkAchievementsModal = useShouldShowAchievementSuccessModal() | ||
const checkReactionModal = useIncomingReactionModal() | ||
|
||
useEffect(() => { | ||
if (shouldShowAchievementSuccessModal) { | ||
showAchievementModal() | ||
} | ||
}, [shouldShowAchievementSuccessModal, showAchievementModal]) | ||
if (hasShowSessionModal) return | ||
hasShowSessionModal = true | ||
const modal = createRelativeModals( | ||
{ | ||
creator: reactionModal, | ||
check: checkReactionModal, | ||
}, | ||
{ | ||
creator: achievementsModal, | ||
check: checkAchievementsModal, | ||
} | ||
) | ||
|
||
const isReactionFeatureActive = useFeatureFlag(RemoteStoreFeatureFlags.WIP_REACTION_FEATURE) | ||
if (!modal) return | ||
openModal(modal) | ||
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. si on arrive depuis un deep link sur autre page que la home, on ne verra pas les modales parce qu'on n'est pas passé par la home est-ce le comportement voulu ?
Comment on lines
+71
to
+85
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. où sont les tests ? |
||
}, [openModal, checkReactionModal, checkAchievementsModal]) | ||
|
||
useEffect(() => { | ||
if (id) { | ||
|
@@ -139,12 +150,6 @@ export const Home: FunctionComponent = () => { | |
visible={onboardingSubscriptionModalVisible} | ||
dismissModal={hideOnboardingSubscriptionModal} | ||
/> | ||
{isReactionFeatureActive ? <IncomingReactionModalContainer /> : null} | ||
<AchievementSuccessModal | ||
names={achievementsToShow} | ||
visible={visibleAchievementModal} | ||
hideModal={hideAchievementModal} | ||
/> | ||
</React.Fragment> | ||
) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ import { withWebWrapper } from 'features/navigation/RootNavigator/withWebWrapper | |
import { TabNavigationStateProvider } from 'features/navigation/TabBar/TabNavigationStateContext' | ||
import { VenueMapFiltersStackNavigator } from 'features/navigation/VenueMapFiltersStackNavigator/VenueMapFiltersStackNavigator' | ||
import { AccessibilityRole } from 'libs/accessibilityRole/accessibilityRole' | ||
import { ModalRenderer } from 'libs/modals/modal.renderer' | ||
import { modalFactory } from 'libs/modals/modals' | ||
import { useSplashScreenContext } from 'libs/splashscreen' | ||
import { storage } from 'libs/storage' | ||
import { IconFactoryProvider } from 'ui/components/icons/IconFactoryProvider' | ||
|
@@ -108,6 +110,7 @@ export const RootNavigator: React.ComponentType = () => { | |
{/* The components below are those for which we do not want | ||
their rendering to happen while the splash is displayed. */} | ||
{isSplashScreenHidden ? <PrivacyPolicy /> : null} | ||
<ModalRenderer modalFactory={modalFactory} /> | ||
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.
est-ce qu'il n'y aurait pas un risque d'afficher les modales au moment du splash screen ? |
||
</TabNavigationStateProvider> | ||
) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import styled from 'styled-components/native' | |
import { AchievementEnum } from 'api/gen' | ||
import { analytics } from 'libs/analytics' | ||
import LottieView from 'libs/lottie' | ||
import { achievementsModal, modalFactory } from 'libs/modals/modals' | ||
import { ButtonPrimary } from 'ui/components/buttons/ButtonPrimary' | ||
import { ButtonTertiaryBlack } from 'ui/components/buttons/ButtonTertiaryBlack' | ||
import { AppInformationModal } from 'ui/components/modals/AppInformationModal' | ||
|
@@ -21,6 +22,10 @@ interface Props { | |
names: AchievementEnum[] | ||
} | ||
|
||
modalFactory.add(achievementsModal, ({ params: { names }, close }) => { | ||
return <AchievementSuccessModal visible hideModal={close} names={names} /> | ||
}) | ||
|
||
export const AchievementSuccessModal = ({ visible, hideModal, names }: Props) => { | ||
const logoRef = useRef<LottieView>(null) | ||
|
||
|
@@ -33,7 +38,7 @@ export const AchievementSuccessModal = ({ visible, hideModal, names }: Props) => | |
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [visible]) | ||
|
||
if (!visible || names.length <= 0) return null | ||
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. pourquoi on n'a plus besoin de cette condition ? |
||
if (!visible) return null | ||
|
||
const severalAchievementsUnlocked = names.length >= 2 | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,22 @@ | ||||||
import { modalStore } from '../modal.store' | ||||||
|
||||||
describe('Feature: Close modal', () => { | ||||||
test('Modal is closed', () => { | ||||||
const modal = { key: 'modal-key', params: { text: 'Hello world!' } } | ||||||
modalStore.setState({ modalOpened: modal }) | ||||||
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. cet état ne devrait pas etre possible : d'après le typage actuel, il y a toujours une queue meme si elle est vide si on passe par les actions exposées par le module, ça devrait etre ok
Suggested change
|
||||||
|
||||||
modalStore.getState().actions.closeModal() | ||||||
|
||||||
expect(modalStore.getState().modalOpened).toBeUndefined() | ||||||
}) | ||||||
|
||||||
test('Modal is opened from queue if there is one', () => { | ||||||
const modal = { key: 'modal-key', params: { text: 'Hello world!' } } | ||||||
const queuedModal = { key: 'queued-modal-key', params: { text: 'Hello world!' } } | ||||||
modalStore.setState({ modalOpened: modal, queue: [queuedModal] }) | ||||||
|
||||||
modalStore.getState().actions.closeModal() | ||||||
|
||||||
expect(modalStore.getState().modalOpened).toEqual(queuedModal) | ||||||
}) | ||||||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import { createModal, createRelativeModals } from '../modal.creator' | ||
|
||
describe('Feature: Modal relative', () => { | ||
test('Modal is returned', () => { | ||
const firstModal = createModal<{ title: string }>('first') | ||
const relativeModal = createRelativeModals({ | ||
creator: firstModal, | ||
check() { | ||
return firstModal({ title: 'hello world' }) | ||
}, | ||
}) | ||
|
||
expect(relativeModal).toEqual(firstModal({ title: 'hello world' })) | ||
}) | ||
|
||
test('Second modal is returned when first modal has not to be open', () => { | ||
const firstModal = createModal<{ title: string }>('first') | ||
const secondModal = createModal<{ name: string }>('second') | ||
const relativeModal = createRelativeModals( | ||
{ | ||
creator: firstModal, | ||
check() { | ||
return undefined | ||
}, | ||
}, | ||
{ | ||
creator: secondModal, | ||
check() { | ||
return secondModal({ name: 'hello world' }) | ||
}, | ||
} | ||
) | ||
|
||
expect(relativeModal).toEqual(secondModal({ name: 'hello world' })) | ||
}) | ||
|
||
test('Return the first modal to be open', () => { | ||
const firstModal = createModal<{ title: string }>('first') | ||
const secondModal = createModal<{ name: string }>('second') | ||
const relativeModal = createRelativeModals( | ||
{ | ||
creator: firstModal, | ||
check() { | ||
return firstModal({ title: 'hello world' }) | ||
}, | ||
}, | ||
{ | ||
creator: secondModal, | ||
check() { | ||
return secondModal({ name: 'hello world' }) | ||
}, | ||
} | ||
) | ||
|
||
expect(relativeModal).toEqual(firstModal({ title: 'hello world' })) | ||
}) | ||
|
||
test('Return undefined when no modal has to be open', () => { | ||
const firstModal = createModal<{ title: string }>('first') | ||
const secondModal = createModal<{ name: string }>('second') | ||
const relativeModal = createRelativeModals( | ||
{ | ||
creator: firstModal, | ||
check() { | ||
return undefined | ||
}, | ||
}, | ||
{ | ||
creator: secondModal, | ||
check() { | ||
return undefined | ||
}, | ||
} | ||
) | ||
|
||
expect(relativeModal).toBeUndefined() | ||
}) | ||
}) |
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.
il n'y a pas de tests ?
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.
la complexité cyclomatique est très élevé : 10