Skip to content
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-34158) feat(remoteBanner): get remoteBanner from firestore #7601

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cgerrard-pass
Copy link
Contributor

@cgerrard-pass cgerrard-pass commented Jan 29, 2025

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

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

image

When input from firestore is incorrect:
image

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

@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch 2 times, most recently from dcd555d to 6cf215e Compare January 30, 2025 16:16
Copy link
Contributor

@xlecunff-pass xlecunff-pass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solide 😎

@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch 3 times, most recently from 7779174 to cb8bd40 Compare January 30, 2025 16:37
@lbeneston-pass
Copy link
Contributor

lbeneston-pass commented Jan 30, 2025

Il n'y a plus de feature forceUpdate ?
Parce que pour moi il fallait garder forceUpdate pour les pages de forceUpdate qui sont spécifique à cette action ?

Il n'y a pas de tests pour le composant remoteBanner (mais la RemoteBanner est testé dans dans la Home, que je trouve bizarre)

@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch 6 times, most recently from e4b3926 to 187db31 Compare January 31, 2025 15:44
@cgerrard-pass cgerrard-pass changed the title (PC-34158) feat(banner): banner from firestore (PC-34158) feat(remoteBanner): get remoteBanner from firestore Jan 31, 2025
@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch from 187db31 to 409b667 Compare January 31, 2025 15:56
src/features/forceUpdate/helpers/useResetOnMinimalBuild.ts Outdated Show resolved Hide resolved
src/features/remoteBanner/helpers/useRemoteBanner.ts Outdated Show resolved Hide resolved
src/features/remoteBanner/helpers/useRemoteBanner.ts Outdated Show resolved Hide resolved
const { title, subtitle, redirectionUrl, redirectionType } = useRemoteBanner()
const accessibilityLabel =
redirectionUrl && redirectionType === RemoteStoreBannerRedirectionType.EXTERNAL
? `Nouvelle fenêtre\u00a0: ${String(redirectionUrl)}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

il se passe quoi s'il y a redirectionUrl qui est vide / undefined ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je vais rajouter une condition pour ne pas faire appel a openUrl s'il n'y a pas de redirectionUrl pour qu'il ne se passe rien si aucun lien n'est donné

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

niveau a11y, on aura un bouton qui ne fait rien quand on interragit avec

@lbeneston-pass t'en penses quoi ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En théorie il faudrait ne pas avoir de bouton ou alors le rendre disabled sans changement de style ce qui serait plus simple ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d'ailleurs, ça devrait etre un bouton ou un lien ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si c'est une redirection externe : un lien
Si c'est une redirection interne / action dans l'app : un bouton

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Donc on peut conditionner le type de composant à redirectionType

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si c'est une redirection interne / action dans l'app : un bouton

il me semble qu'en web, ça devrait etre un lien

dans ce cas, je pense que ça devrait etre le role de InternalTouchableLink qui détermine le type d'élément en fonction de la plateforme

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch 3 times, most recently from 934c4f8 to 3363b10 Compare February 4, 2025 15:12
@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch 7 times, most recently from c54d30f to 494329b Compare February 6, 2025 17:38
@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch 2 times, most recently from 82c8df3 to f4d4efa Compare February 6, 2025 21:17
@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch 3 times, most recently from a495905 to bcb5090 Compare February 7, 2025 16:05
@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch from 8a10190 to a577adb Compare February 7, 2025 17:40
@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch from a577adb to 67e0ded Compare February 8, 2025 11:53
@cgerrard-pass cgerrard-pass force-pushed the PC-34158-banner-from-firestore branch from 7688d53 to cb890da Compare February 9, 2025 14:50
Copy link

sonarqubecloud bot commented Feb 9, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants