(PC-33485) feat(Modals): resolve modal conflicts #7427

mleduc-pass · 2024-12-19T05:30:50Z

Link to JIRA ticket: https://passculture.atlassian.net/browse/PC-33485

Flakiness

If I had to re-run tests in the CI due to flakiness, I add the incident on Notion

Checklist

I have:

Made sure my feature is working on web.
Made sure my feature is working on mobile (depending on relevance : real or virtual devices)
Written unit tests native (and web when implementation is different) for my feature.
Added a screenshot for UI tickets or deleted the screenshot section if no UI change
If my PR is a bugfix, I add the link of the "résolution de problème sur le bug" on Notion
I am aware of all the best practices and respected them.

Best Practices

Click to expand

These rules apply to files that you make changes to. If you can't respect one of these rules, be sure to explain why with a comment. If you consider correcting the issue is too time consuming/complex: create a ticket. Link the ticket in the code.

In the production code: remove type assertions with as (type assertions are removed at compile-time, there is no runtime checking associated with a type assertion. There won’t be an exception or null generated if the type assertion is wrong). In certain cases as const is acceptable (for example when defining readonly arrays/objects). Using as in tests is tolerable.
Remove bypass type checking with any (when you want to accept anything because you will be blindly passing it through without interacting with it, you can use unknown). Using any in tests is tolerable.
Remove non-null assertion operators (just like other type assertions, this doesn’t change the runtime behavior of your code, so it’s important to only use ! when you know that the value can’t be null or undefined).
Remove all @ts-expect-error and @eslint-disable.
Remove all warnings, and errors that we are used to ignore (yarn test:lint, yarn test:types, yarn start:web...).
Use gap (ViewGap) instead of <Spacer.Column />, <Spacer.Row /> or <Spacer.Flex />.
Don't add new "alias hooks" (hooks created to group other hooks together). When adding new logic, this hook will progressively become more complex and harder to maintain.
Remove logic from components that should be dumb.

Test specific:

Avoid mocking internal parts of our code. Ideally, mock only external calls.
When you see a local variable that is over-written in every test, mock it.
Prefer user to fireEvent.
When mocking feature flags, use setFeatureFlags. If not possible, mention which one(s) you want to mock in a comment (example: jest.spyOn(useFeatureFlagAPI, 'useFeatureFlag').mockReturnValue(true) // WIP_NEW_OFFER_TILE in renderPassPlaylist.tsx )
In component tests, replace await act(async () => {}) and await waitFor(/* ... */) by await screen.findBySomething().
In hooks tests, use act by default and waitFor as a last resort.
Make a snapshot test for pages and modals ONLY.
Make a web specific snapshot when your web page/modal is specific to the web.
Make an a11y test for web pages.

Advice:

Use TDD
Use Storybook
Use pair programming/mobs

bebstein-pass

Merci pour cette conception qui a du demander beaucoup de réflexion

Il va falloir documenter (dans /doc)

si j'ai bien tout compris (c'est encore confus pour moi), on a :

des modales qui sont composées d'une clé et de props
un store Zustand qui gère une file d'attente (non priorisée) de modales
une map qui stock / donne le composant pour render une modale couplé à une clé de modale

est-ce que je me trompe ?

bebstein-pass · 2024-12-19T14:17:51Z

src/libs/modals/modals.factory.ts

+      if (modals.has(modalCreator.key)) {
+        throw new Error(`Modal with key\u00a0: ${modalCreator.key} Already exist`)
+      }
+      modals.set(modalCreator.key, element as React.FC<{ params: unknown }>)


récemment, notre communauté de dev s'est mise d'accord par consensus pour éviter d'ajouter des as dans la codebase (et supprimer les existants)

si jamais on pense ne pas avoir le choix, il faudrait ajouter un commentaire disant pourquoi c'est nécessaire à cet endroit et qu'est-ce qu'on a essayé de faire comme solution alternative

bebstein-pass · 2024-12-19T14:23:04Z

src/libs/modals/modals.ts

+export const achievementsModal = createModal<{ names: AchievementEnum[] }>('achievements')
+
+// reactionModal à réadapter
+export const reactionModal = createModal<{


ce fichier semble etre une lib très générique (d'après son chemin)

ce fichier dépend d'une feature très spécifique (reaction)

est-ce qu'à terme, ce fichier contiendra toutes les modales de l'app ?

quels seront les impacts sur du code splitting ?

ce fichier ne réutilise pas les props défini dans les composants des modals

pass-culture-app-native/src/features/reactions/components/ReactionChoiceModal/ReactionChoiceModal.tsx

Line 29 in 99d132e

type Props = {

avec les évolutions futures, il risque d'y avoir une divergeance entre le type fourni à createModal et le type des props du composant

bebstein-pass · 2024-12-19T14:27:26Z

src/libs/modals/modal.creator.ts

+      params,
+    }
+  }
+  creator.key = key


pourquoi est-ce nécessaire ?

bebstein-pass · 2024-12-19T14:55:50Z

src/libs/modals/modal.store.ts

+      const hasModalOpened = state.modalOpened !== undefined
+
+      return {
+        ...state,


pourquoi a-t-on besoin de spread le state ?

bebstein-pass · 2024-12-19T14:56:33Z

src/libs/modals/modal.store.ts

+      const [nextModal] = state.queue
+
+      return {
+        ...state,
+        modalOpened: nextModal,
+        queue: state.queue.filter((queuedModal) => queuedModal !== nextModal),
+      }


est-ce que ceci serait valide ? si oui, ça me semble plus simple

Suggested change

const [nextModal] = state.queue

return {

...state,

modalOpened: nextModal,

queue: state.queue.filter((queuedModal) => queuedModal !== nextModal),

}

const [nextModal, ...queue] = state.queue

return {

...state,

modalOpened: nextModal,

queue,

}

bebstein-pass · 2024-12-19T17:39:48Z

src/features/home/components/IncomingReactionModalContainer/IncomingReactionModalContainer.tsx

+  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


si cette partie était faite dans getModal, on pourrait faire des early return, ce qui permettrait de supprimer plein de ?

ça pourrait éviter quelques calculs dans le cas où une autre modal s'affiche avant celle ci

bebstein-pass · 2024-12-19T17:49:25Z

src/features/home/pages/Home.tsx


-  const isReactionFeatureActive = useFeatureFlag(RemoteStoreFeatureFlags.WIP_REACTION_FEATURE)
+    if (!modal) return
+    openModal(modal)


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 ?

bebstein-pass · 2024-12-19T17:52:19Z

src/libs/modals/modal.renderer.tsx

+    return null
+  }
+
+  const ModalRenderer = modalFactory.get(modalOpened)


le composant s'appelle aussi ModalRenderer

le shadowing de variable est souvent source de confusion

je suggère de renommer soit le composant, soit la variable dans la fonction

bebstein-pass · 2024-12-19T18:03:42Z

src/features/home/pages/Home.tsx

+    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)


où sont les tests ?

bebstein-pass · 2024-12-19T18:05:13Z

src/features/navigation/RootNavigator/RootNavigator.tsx

@@ -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} />


PrivacyPolicy s'affiche uniquement si le splash screen n'est plus affiché

est-ce qu'il n'y aurait pas un risque d'afficher les modales au moment du splash screen ?

pcanthelou-pass · 2025-01-17T09:31:30Z

@bebstein-pass Que faire de cette PR ?

mleduc-pass added 5 commits December 18, 2024 14:13

Feature: open/close modal in store

68c5f31

Use modal system for achievements modal

0e1371e

Refactor

1b39457

Remove unused actions

cd0b049

Display reaction modal or achievements modal on first session

9bc0335

bebstein-pass reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PC-33485) feat(Modals): resolve modal conflicts #7427

(PC-33485) feat(Modals): resolve modal conflicts #7427

mleduc-pass commented Dec 19, 2024

bebstein-pass left a comment

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

bebstein-pass Dec 19, 2024

pcanthelou-pass commented Jan 17, 2025

(PC-33485) feat(Modals): resolve modal conflicts #7427

Are you sure you want to change the base?

(PC-33485) feat(Modals): resolve modal conflicts #7427

Conversation

mleduc-pass commented Dec 19, 2024

Flakiness

Checklist

Best Practices

bebstein-pass 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

pcanthelou-pass commented Jan 17, 2025