-
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
(PC-34158) feat(remoteBanner): get remoteBanner from firestore #7601
base: master
Are you sure you want to change the base?
Conversation
dcd555d
to
6cf215e
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.
Solide 😎
7779174
to
cb8bd40
Compare
Il n'y a plus de feature forceUpdate ? Il n'y a pas de tests pour le composant remoteBanner (mais la RemoteBanner est testé dans dans la Home, que je trouve bizarre) |
e4b3926
to
187db31
Compare
187db31
to
409b667
Compare
src/features/forceUpdate/helpers/useMustUpdateApp.native.test.ts
Outdated
Show resolved
Hide resolved
src/features/remoteBanner/helpers/useRemoteBanner.native.test.ts
Outdated
Show resolved
Hide resolved
src/features/remoteBanner/helpers/useRemoteBanner.native.test.ts
Outdated
Show resolved
Hide resolved
const { title, subtitle, redirectionUrl, redirectionType } = useRemoteBanner() | ||
const accessibilityLabel = | ||
redirectionUrl && redirectionType === RemoteStoreBannerRedirectionType.EXTERNAL | ||
? `Nouvelle fenêtre\u00a0: ${String(redirectionUrl)}` |
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.
il se passe quoi s'il y a redirectionUrl
qui est vide / undefined ?
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 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é
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.
niveau a11y, on aura un bouton qui ne fait rien quand on interragit avec
@lbeneston-pass t'en penses quoi ?
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.
En théorie il faudrait ne pas avoir de bouton ou alors le rendre disabled sans changement de style ce qui serait plus simple ?
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.
d'ailleurs, ça devrait etre un bouton ou un lien ?
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.
Si c'est une redirection externe : un lien
Si c'est une redirection interne / action dans l'app : un bouton
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.
Donc on peut conditionner le type de composant à redirectionType
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.
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
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.
Ticket pour l'a11y https://passculture.atlassian.net/browse/PC-34485
934c4f8
to
3363b10
Compare
src/features/forceUpdate/helpers/useMustUpdateApp.native.test.ts
Outdated
Show resolved
Hide resolved
c54d30f
to
494329b
Compare
src/libs/firebase/firestore/featureFlags/__tests__/setFeatureFlags.tsx
Outdated
Show resolved
Hide resolved
82c8df3
to
f4d4efa
Compare
a495905
to
bcb5090
Compare
8a10190
to
a577adb
Compare
a577adb
to
67e0ded
Compare
7688d53
to
cb890da
Compare
|
|
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:
Screenshots
When input from firestore is incorrect:
![image](https://private-user-images.githubusercontent.com/187540148/411321831-f110904a-0096-4987-9e2a-5495d4881f09.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMjU0NjIsIm5iZiI6MTczOTEyNTE2MiwicGF0aCI6Ii8xODc1NDAxNDgvNDExMzIxODMxLWYxMTA5MDRhLTAwOTYtNDk4Ny05ZTJhLTU0OTVkNDg4MWYwOS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjA5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIwOVQxODE5MjJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT05NDBiZWI5YTJlZTZmZjlmZTZhMDg5ZTE0ZjBhZDMxMWRiMzkzYjNjMjM4YTNhZTU2N2Y4OGEyZGQxYzA1MzE3JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.1YccZ9si614hRRwXBKIb2Esq7gWmoN8biPG3DnzSVyU)
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: