(PC-34196) feat(carousel): create custom carousel to replace carousel lib #7603

tprache-pass · 2025-01-29T17:26:25Z

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

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.

Screenshots

delete if no UI change

Platform	Mockup/Before	After
iOS		Simulator.Screen.Recording.-.iPhone.13.mini.-.2025-01-29.at.18.07.14.mp4
Android
Phone - Chrome	Enregistrement.de.l.ecran.2025-01-29.a.18.12.19.mov	Enregistrement.de.l.ecran.2025-01-29.a.18.09.59.mov
Desktop - Chrome	Enregistrement.de.l.ecran.2025-01-29.a.18.12.42.mov	Enregistrement.de.l.ecran.2025-01-29.a.18.09.22.mov

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

nmajorfrances-pass · 2025-02-19T14:33:18Z

src/features/offer/components/OfferContent/OfferContent.web.test.tsx

@@ -83,7 +83,8 @@ function renderOfferContent({
  )
 }

-describe('<OfferContent />', () => {
+// TODO(PC-34650) : react-native-web bump needed becasue of "setNativeProps is deprecated" warning making the test to fail
+describe.skip('<OfferContent />', () => {


eslint.disable ?

nmajorfrances-pass · 2025-02-19T14:36:17Z

src/features/offer/components/OfferImageContainer/OfferImageContainer.web.test.tsx

@@ -8,7 +8,8 @@ jest.mock('libs/firebase/remoteConfig/remoteConfig.services')

 const mockOnPress = jest.fn()

-describe('<OfferImageContainer />', () => {
+// TODO(PC-34650) : react-native-web bump needed becasue of "setNativeProps is deprecated" warning making the test to fail
+describe.skip('<OfferImageContainer />', () => {


eslint.disable ?

nmajorfrances-pass · 2025-02-19T14:37:59Z

src/features/offer/pages/Offer/Offer.web.test.tsx

@@ -60,7 +60,8 @@ jest.mock('api/useSearchVenuesOffer/useSearchVenueOffers', () => ({
 jest.mock('libs/firebase/analytics/analytics')
 jest.mock('libs/firebase/remoteConfig/remoteConfig.services')

-describe('<Offer/>', () => {
+// TODO(PC-34650) : react-native-web bump needed becasue of "setNativeProps is deprecated" warning making the test to fail
+describe.skip('<Offer/>', () => {


eslint.disable ?

nmajorfrances-pass · 2025-02-19T14:39:27Z

src/ui/components/Carousel/Carousel.tsx

+
+type CarouselProps = {
+  data: unknown[]
+  // We use any on purpose in this file


oui mais pourquoi ?

imouandjolobe-pass · 2025-02-20T09:45:21Z

...s/offer/components/OfferImageCarouselPagination/OfferImageCarouselPagination.native.test.tsx

  it('should not display pagination with dots and buttons', () => {
    render(
      <OfferImageCarouselPagination
-        progressValue={{ value: 0 }}
+        progressValue={PROGRESS_VALUE}
        offerImages={['image1', 'image2']}
        handlePressButton={jest.fn()}
      />


Je ne comprends pas la pertinence de ce test, j'ai l'impression que l'on veut tester dans un test natif que l'on n'affiche pas le composant web ?
Ton premier test me semble suffisant 🙂

imouandjolobe-pass · 2025-02-20T09:45:25Z

...ures/offer/components/OfferImageCarouselPagination/OfferImageCarouselPagination.web.test.tsx

  it('should not display pagination with only dots', () => {
    render(
      <OfferImageCarouselPagination
-        progressValue={{ value: 0 }}
+        progressValue={PROGRESS_VALUE}
        offerImages={['image1', 'image2']}
        handlePressButton={mockHandlePressButton}
      />


Pareil ici, ce test semble vérifier que l'on affiche pas le composant natif, je pense que l'on peut s'en passer 🙂

imouandjolobe-pass · 2025-02-20T10:02:04Z

src/ui/components/Carousel/Carousel.tsx

+  const onViewableItemsChanged = ({
+    viewableItems,
+  }: {
+    changed: ViewToken[]


cette props ne semble plus être d'actualité 🙂

imouandjolobe-pass · 2025-02-20T10:02:35Z

src/ui/components/Carousel/Carousel.tsx

+      bounces={false}
+      pagingEnabled
+      viewabilityConfigCallbackPairs={
+        viewabilityConfigCallbackPairs.current as ViewabilityConfigCallbackPairs


j'ai l'impression que l'on peut se passer de as 🤔

imouandjolobe-pass · 2025-02-20T10:15:15Z

bpeyrou-pass · 2025-02-20T13:56:35Z

src/ui/components/Carousel/Carousel.tsx

+  data: unknown[]
+  // We use any on purpose in this file
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  renderItem: ({ item, index }: { item: any; index: number }) => any


en utilisant les alias de types on peut pas enlever les "any" ?

github-actions · 2025-02-24T13:02:17Z

Visit the preview URL for this PR (updated for commit 544458b):

https://pc-native-testing--pr7603-pc-34196-replace-car-3p3y499k.web.app

_{(expires Thu, 27 Feb 2025 13:02:14 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 54dd253190f9fbd3911a4f3fbe0f3af45c56e75f}

sonarqubecloud · 2025-02-24T13:07:54Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sonarqubecloud · 2025-02-24T13:13:30Z

Quality Gate failed

Failed conditions
80.0% Coverage on New Code (required ≥ 85%)

See analysis details on SonarQube Cloud

tprache-pass force-pushed the pc-34196/replace-carousel branch 2 times, most recently from 4328791 to f08c2cc Compare February 14, 2025 16:14

tprache-pass marked this pull request as ready for review February 14, 2025 16:15

tprache-pass force-pushed the pc-34196/replace-carousel branch from 8c34140 to d02ca97 Compare February 17, 2025 14:12

tprache-pass added the preview Triggers a deployment to testing (firebase) label Feb 17, 2025

nmajorfrances-pass reviewed Feb 19, 2025

View reviewed changes

imouandjolobe-pass approved these changes Feb 20, 2025

View reviewed changes

bpeyrou-pass reviewed Feb 20, 2025

View reviewed changes

tprache-pass added 11 commits February 24, 2025 09:59

build: bump reanimated and remove carousel lib

759cefb

feat: create custom carousel

4e0c9ce

refactor: replace carousel for offer images

46db0bc

refactor: replace carousel for video module

7310cf9

fix: add config for jest after reanimated bump

ba16ecf

test: update tests

b992a9e

feat: carousel optimisation

202f465

fix: fix code smells

91c8cd2

test: skip failing tests

36fe43e

test: add carousel test

e7c9be2

fix: take PR review comments into account

544458b

tprache-pass force-pushed the pc-34196/replace-carousel branch from d02ca97 to 544458b Compare February 24, 2025 12:58

nmajorfrances-pass approved these changes Feb 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PC-34196) feat(carousel): create custom carousel to replace carousel lib #7603

(PC-34196) feat(carousel): create custom carousel to replace carousel lib #7603

tprache-pass commented Jan 29, 2025 •

edited

Loading

nmajorfrances-pass Feb 19, 2025

nmajorfrances-pass Feb 19, 2025

nmajorfrances-pass Feb 19, 2025

nmajorfrances-pass Feb 19, 2025

imouandjolobe-pass Feb 20, 2025

imouandjolobe-pass Feb 20, 2025

imouandjolobe-pass Feb 20, 2025

imouandjolobe-pass Feb 20, 2025

imouandjolobe-pass commented Feb 20, 2025

bpeyrou-pass Feb 20, 2025

github-actions bot commented Feb 24, 2025

sonarqubecloud bot commented Feb 24, 2025

sonarqubecloud bot commented Feb 24, 2025

(PC-34196) feat(carousel): create custom carousel to replace carousel lib #7603

Are you sure you want to change the base?

(PC-34196) feat(carousel): create custom carousel to replace carousel lib #7603

Conversation

tprache-pass commented Jan 29, 2025 • edited Loading

Flakiness

Checklist

Screenshots

Best Practices

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

imouandjolobe-pass commented Feb 20, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2025

sonarqubecloud bot commented Feb 24, 2025

Quality Gate passed

sonarqubecloud bot commented Feb 24, 2025

Quality Gate failed

tprache-pass commented Jan 29, 2025 •

edited

Loading