-
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
(BSR) fix(global): splashscreen - network relations #7445
base: master
Are you sure you want to change the base?
Conversation
96e85fc
to
4bc86f3
Compare
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.
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
src/libs/network/NetInfoWrapper.tsx
Outdated
@@ -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() |
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.
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 ?
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.
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]) |
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.
je ne sais pas si c'est normal de ne pas mettre à jour les dépendances
1c71d61
to
fefbfbe
Compare
.eslintrc.js
Outdated
@@ -317,6 +317,7 @@ module.exports = { | |||
['types', './src/types'], | |||
['ui', './src/ui'], | |||
['web', './src/web'], | |||
['events', './src/events'], |
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.
à mon avis ce bout de doc va t'etre utile https://github.com/pass-culture/pass-culture-app-native/blob/12f3d3e7117ed66562148c4882d342aadc275d45/doc/development/alias.md
5ffeda2
to
b1b2fb1
Compare
Quality Gate failedFailed conditions |
b1b2fb1
to
45a5cb2
Compare
45a5cb2
to
5463f62
Compare
5463f62
to
c695fd9
Compare
Quality Gate passedIssues Measures |
Fix au démarrage de l'app quand on a pas de réseauCette 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:
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.as
(type assertions are removed at compile-time, there is no runtime checking associated with a type assertion. There won’t be an exception ornull
generated if the type assertion is wrong). In certain casesas const
is acceptable (for example when defining readonly arrays/objects). Usingas
in tests is tolerable.any
(when you want to accept anything because you will be blindly passing it through without interacting with it, you can useunknown
). Usingany
in tests is tolerable.!
when you know that the value can’t benull
orundefined
).@ts-expect-error
and@eslint-disable
.yarn test:lint
,yarn test:types
,yarn start:web
...).gap
(ViewGap
) instead of<Spacer.Column />
,<Spacer.Row />
or<Spacer.Flex />
.Test specific:
user
tofireEvent
.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
)await act(async () => {})
andawait waitFor(/* ... */)
byawait screen.findBySomething()
.act
by default andwaitFor
as a last resort.Advice: