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

(BSR) fix(global): splashscreen - network relations #7445

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mmeissonnier-pass
Copy link
Contributor

@mmeissonnier-pass mmeissonnier-pass commented Dec 24, 2024

Fix au démarrage de l'app quand on a pas de réseau
Cette PR s'est un peu transformée en PR de refacto 😅

  • Implémentation d'un store global qui permet de pouvoir gérer de façon plus élégante les interactions entre le splashscreen, le network et la navigation.

  • L'affichage de l'écran d'erreur de connexion est directement géré au niveau du provider NetInfoWrapper (pas de mode offline possible du coup. Problématique ? Si oui quel intérêt d'avoir un écran d'erreur dédié 🤔 )

  • Suppression d'une mécanique de temporisation sur l'affichage du splashscreen .

-Suppression du context dédié au splashscreen

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

Avant

Kapture.2024-12-24.at.10.44.58.mp4

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

@mmeissonnier-pass mmeissonnier-pass force-pushed the fix/BSR-no-network-at-startup branch from 96e85fc to 4bc86f3 Compare December 24, 2024 13:33
@mmeissonnier-pass mmeissonnier-pass marked this pull request as ready for review December 24, 2024 13:52
@mmeissonnier-pass mmeissonnier-pass marked this pull request as draft December 24, 2024 14:02
Copy link
Contributor

@bebstein-pass bebstein-pass left a comment

Choose a reason for hiding this comment

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

j'aime quand on évite de spammer des erreurs


sur un appareil peu performant, l'app peut mettre très longtemps à charger

avec cette modification, on ne sait pas si c'est juste très lent à charger ou s'il y a un problème (réseau non disponible)

s'il y avait un toast ou autre truc qui prévenait qu'il n'y a pas de réseau, je serais confiant

je désactive mon réseau quand je dors, j'utilise peu mon téléphone le matin, et donc parfois au milieu de la journée, je n'ai toujours pas réactivé le réseau, parfois il m'arrive de lancer une app et de ne pas tout de suite comprendre pourquoi ça ne fonctionne pas

j'aimerais bien que ça soit validé coté UX, car maintenant nos jeunes ne sont plus prévenu que l'app ne charge pas parce qu'il n'y a pas de réseau

@@ -15,9 +16,10 @@ export const NetInfoWrapper = memo(function NetInfoWrapper({
}) {
const networkInfo = useNetInfo()
const { showInfoSnackBar } = useSnackBarContext()
const isConnected = !!networkInfo.isConnected && !!networkInfo.isInternetReachable
const { isSplashScreenHidden } = useSplashScreenContext()
Copy link
Contributor

Choose a reason for hiding this comment

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

je trouve ça bizarre d'avoir une dépendance vers le splashscreen depuis le
truc qui gère le réseau

est-ce qu'on pourrait le mettre ailleur ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oui je suis d'accord. Il faudra que je trouve un moyen de séparer ça.

@@ -43,7 +45,11 @@ export const NetInfoWrapper = memo(function NetInfoWrapper({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [networkInfo.type])
Copy link
Contributor

Choose a reason for hiding this comment

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

je ne sais pas si c'est normal de ne pas mettre à jour les dépendances

@mmeissonnier-pass mmeissonnier-pass force-pushed the fix/BSR-no-network-at-startup branch 8 times, most recently from 1c71d61 to fefbfbe Compare January 8, 2025 10:28
.eslintrc.js Outdated
@@ -317,6 +317,7 @@ module.exports = {
['types', './src/types'],
['ui', './src/ui'],
['web', './src/web'],
['events', './src/events'],
Copy link
Contributor

Choose a reason for hiding this comment

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

@mmeissonnier-pass mmeissonnier-pass force-pushed the fix/BSR-no-network-at-startup branch 2 times, most recently from 5ffeda2 to b1b2fb1 Compare January 8, 2025 13:06
Copy link

sonarqubecloud bot commented Jan 8, 2025

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

@mmeissonnier-pass mmeissonnier-pass changed the title (BSR) fix(global): keep splashscreen when no network at startup (BSR) fix(global): splashscreen - network relationship Jan 9, 2025
@mmeissonnier-pass mmeissonnier-pass force-pushed the fix/BSR-no-network-at-startup branch from b1b2fb1 to 45a5cb2 Compare January 9, 2025 18:53
@mmeissonnier-pass mmeissonnier-pass force-pushed the fix/BSR-no-network-at-startup branch from 45a5cb2 to 5463f62 Compare January 9, 2025 19:00
@mmeissonnier-pass mmeissonnier-pass changed the title (BSR) fix(global): splashscreen - network relationship (BSR) fix(global): splashscreen - network relations Jan 9, 2025
@mmeissonnier-pass mmeissonnier-pass force-pushed the fix/BSR-no-network-at-startup branch from 5463f62 to c695fd9 Compare January 9, 2025 20:04
Copy link

sonarqubecloud bot commented Jan 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.

2 participants