feat: Better Profile Modal #658
Conversation
WalkthroughRemplacement de la navigation/back par un auto-save via useNavigation (beforeRemove) dans le modal profil ; image picker cadrée 1:1, avatar agrandi, centrement du contenu, traductions des libellés et petits ajustements UI/spacing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProfileModal
participant ImagePicker
participant Navigation
participant AccountService
User ->> ProfileModal: ouvre modal profil
User ->> ProfileModal: édite prénom/nom, modifie avatar
ProfileModal ->> ImagePicker: ouvre sélecteur d'image (mediaTypes: images, aspect 1:1)
ImagePicker -->> ProfileModal: retourne image sélectionnée
User ->> Navigation: déclenche fermeture/back
Navigation ->> ProfileModal: beforeRemove
ProfileModal ->> AccountService: updateAccountName(noms trimés)
AccountService -->> ProfileModal: réponse (succès/erreur)
ProfileModal ->> Navigation: autorise fermeture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/(modals)/profile.tsx (2)
150-150: Utiliser i18n pour les labels.Les labels "Prénom" et "Nom" sont codés en dur alors que le reste du composant utilise
t()pour l'internationalisation. Pour maintenir la cohérence et supporter plusieurs langues, ces chaînes devraient utiliser le système de traduction.🌍 Correction proposée pour l'internationalisation
- <Typography color="secondary">Prénom</Typography> + <Typography color="secondary">{t("Label_FirstName")}</Typography> <OnboardingInput - placeholder={"Prénom"} + placeholder={t("Label_FirstName")} text={firstName} setText={setFirstName} icon={"Font"} inputProps={{}} /> - <Typography color="secondary">Nom</Typography> + <Typography color="secondary">{t("Label_LastName")}</Typography> <OnboardingInput - placeholder={"Nom"} + placeholder={t("Label_LastName")} text={lastName} setText={setLastName} icon={"Bold"} inputProps={{}} />Also applies to: 159-159
170-181: Problème d'architecture : NativeHeaderSide placé dans le ScrollView.Le composant
NativeHeaderSideest placé à l'intérieur duScrollView, ce qui est incorrect. Les en-têtes de navigation doivent être :
- Fixes et toujours visibles : Placés en dehors du contenu défilable
- En haut de l'écran : Pas après le contenu du formulaire
- Gérés par le framework de navigation : Typiquement via les options de navigation de
expo-routerAvec le placement actuel :
- Le bouton de retour défile avec le contenu et peut être invisible
- Il apparaît en bas du formulaire au lieu de la position attendue en haut à gauche
- Cela va à l'encontre des conventions UX pour les contrôles de navigation
🔧 Solution recommandée
Déplacez la logique de sauvegarde dans un
useEffectou utilisez les options de navigation d'expo-routerpour définir le bouton de retour dans l'en-tête réel :// Option 1 : Sauvegarder automatiquement avec useEffect useEffect(() => { // Sauvegarder les changements quand le composant se démonte return () => { if (firstName || lastName) { useAccountStore.getState().setAccountName(lastUsedAccount, firstName, lastName); } }; }, [firstName, lastName, lastUsedAccount]); // Option 2 : Utiliser les options de navigation (dans le layout ou avec useNavigation) // Configurez le bouton de retour via expo-router navigation optionsEnsuite, supprimez le bloc
NativeHeaderSideduScrollView.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/(modals)/profile.tsxcomponents/onboarding/OnboardingInput.tsx
🔇 Additional comments (5)
components/onboarding/OnboardingInput.tsx (1)
8-8: Ajustements de style approuvés.La réduction du
paddingVerticalde 10 à 5 pour Android répond à l'objectif du commit "Too big on Android". Les modifications de formatage n'ont aucun impact fonctionnel.Also applies to: 30-31
app/(modals)/profile.tsx (4)
53-53: Bon choix pour le ratio d'image.Le passage à un ratio carré (1:1) est approprié pour les photos de profil et correspond aux objectifs du PR.
78-78: Correction appropriée du comportement du clavier.Le changement de
keyboardVerticalOffsetde-insets.top * 3.2versinsets.bottomcorrige le problème mentionné dans les objectifs du PR où le texte d'entrée était masqué. L'utilisation deinsets.bottomest appropriée pourbehavior="position".
84-84: Améliorations de mise en page approuvées.Les modifications de mise en page (centrage vertical avec
flexGrow: 1etjustifyContent: "center", suppression dupaddingTop, et augmentation de la taille de l'avatar à 150) correspondent bien aux objectifs du PR d'améliorer l'interface du modal de profil.Also applies to: 86-89
51-51: Aucune action requise – le paramètremediaTypes: "images"est correct.La valeur
"images"est un littéral de chaîne valide accepté par l'APIexpo-image-pickerv17.0.8. La documentation officielle indique quemediaTypesaccepte uneMediaType('images' | 'videos' | 'livePhotos') ou un tableau deMediaType. L'utilisation d'une chaîne au lieu d'un tableau est appropriée et conforme à la spécification de l'API.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/(modals)/profile.tsx (2)
170-181: Problème d'architecture :NativeHeaderSidene devrait pas être dans leScrollView.Le composant
NativeHeaderSideest placé à l'intérieur duScrollView, ce qui est architecturalement incorrect. Les en-têtes de navigation doivent être fixes et situés en dehors du contenu défilable, sinon ils défileront avec le contenu.De plus, la prop
key={firstName}-${lastName}provoquera un remontage du composant à chaque changement de nom, ce qui peut causer des problèmes de performance et de perte de focus.🔧 Correction proposée
Le
NativeHeaderSidedevrait être déplacé en dehors duScrollView, probablement au niveau du composant parent ou en utilisant un conteneurViewqui englobe à la fois l'en-tête et le contenu défilable :<KeyboardAvoidingView behavior={"position"} keyboardVerticalOffset={insets.bottom} style={{ flex: 1 }} > + <NativeHeaderSide side="Left"> + <NativeHeaderPressable + onPressIn={() => { + useAccountStore.getState().setAccountName(lastUsedAccount, firstName, lastName); + router.back(); + }} + > + <Icon papicon size={26}> + <Papicons name="ArrowLeft" /> + </Icon> + </NativeHeaderPressable> + </NativeHeaderSide> <ScrollView contentInsetAdjustmentBehavior="automatic" style={{ height: "100%" }} contentContainerStyle={{ flexGrow: 1, justifyContent: "center" }} > {/* ... content ... */} - <NativeHeaderSide side="Left" key={`${firstName}-${lastName}`}> - <NativeHeaderPressable - onPressIn={() => { - useAccountStore.getState().setAccountName(lastUsedAccount, firstName, lastName); - router.back(); - }} - > - <Icon papicon size={26}> - <Papicons name="ArrowLeft" /> - </Icon> - </NativeHeaderPressable> - </NativeHeaderSide> </ScrollView> </KeyboardAvoidingView>
183-183: Erreur de syntaxe dans la balise de fermeture.Il y a un espace dans la balise de fermeture
</ KeyboardAvoidingView >qui causera une erreur de compilation. Les balises JSX ne doivent pas avoir d'espaces entre</et le nom du composant.🐛 Correction proposée
- </ KeyboardAvoidingView > + </KeyboardAvoidingView>
🧹 Nitpick comments (3)
app/(modals)/profile.tsx (3)
87-87: LepaddingHorizontal: 50pourrait être problématique sur les petits écrans.Une marge horizontale de 50px de chaque côté (100px au total) peut rendre l'interface trop étroite sur les petits appareils. Sur un écran de 320px de large, il ne reste que 220px pour l'Avatar de 150px et les autres éléments. Envisagez de réduire cette valeur à 20-30 ou d'utiliser un pourcentage.
♻️ Suggestion de refactoring
- <View style={{ paddingHorizontal: 50, alignItems: "center", gap: 15 }}> + <View style={{ paddingHorizontal: 20, alignItems: "center", gap: 15 }}>
150-159: Utiliser les traductions pour les labels "Prénom" et "Nom".Les chaînes "Prénom" et "Nom" sont codées en dur, alors que le reste du fichier utilise
t()pour les traductions. Pour la cohérence et la prise en charge de l'internationalisation, utilisez des clés de traduction.♻️ Correction proposée
- <Typography color="secondary">Prénom</Typography> + <Typography color="secondary">{t("Label_FirstName")}</Typography> <OnboardingInput placeholder={t("Label_FirstName")} text={firstName} setText={setFirstName} icon={"Font"} inputProps={{}} /> - <Typography color="secondary">Nom</Typography> + <Typography color="secondary">{t("Label_LastName")}</Typography> <OnboardingInput placeholder={t("Label_LastName")}
51-51: Considérer l'utilisation de l'énumérationImagePicker.MediaTypeOptions.Imagesau lieu de la chaîne"images".La valeur
"images"est valide et fonctionne correctement (c'est même la valeur par défaut dans expo-image-picker v17.0.8), mais utiliser l'énumérationMediaTypeOptions.Imagesoffre une meilleure clarté et une sécurité de type renforcée.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(modals)/profile.tsx
🔇 Additional comments (2)
app/(modals)/profile.tsx (2)
78-78: Bonne correction du comportement du clavier !Le changement de
keyboardVerticalOffsetde-insets.top * 3.2versinsets.bottomcorrige le problème d'inversion du clavier mentionné dans la PR. Utiliserinsets.bottomest l'approche correcte pour gérer le décalage du clavier.
84-84: Vérifier le comportement du centrage avec le clavier actif.L'ajout de
justifyContent: "center"avecflexGrow: 1centre bien le contenu, mais peut causer des problèmes lorsque le clavier est visible ou sur les petits écrans. Le contenu pourrait être poussé hors de l'écran. Testez sur différentes tailles d'écran avec le clavier ouvert.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/(modals)/profile.tsx (3)
170-181: Problème architectural critique : bouton de retour dans le ScrollView.Le composant
NativeHeaderSideest placé à l'intérieur duScrollView, ce qui signifie que le bouton de retour défilera avec le contenu et pourra disparaître de l'écran. Cela crée une mauvaise expérience utilisateur car :
- L'utilisateur peut perdre l'accès au bouton de retour en faisant défiler
- Les éléments d'en-tête doivent être fixes et toujours accessibles
- Cela va à l'encontre des conventions UI/UX mobiles
Solutions recommandées :
Option 1 (Recommandée) : Utiliser l'API de navigation native pour définir l'en-tête en dehors du ScrollView.
Option 2 : Si vous devez gérer l'en-tête manuellement, déplacez-le en dehors du
KeyboardAvoidingViewou utilisez une structure de layout avec position absolue.🔧 Proposition de correction avec l'API de navigation
Retirez le
NativeHeaderSidede l'intérieur du ScrollView et configurez plutôt l'en-tête via les options de navigation :export default function CustomProfileScreen() { const { t } = useTranslation(); + + // Configure l'en-tête avec l'action de sauvegarde + React.useLayoutEffect(() => { + navigation.setOptions({ + headerLeft: () => ( + <NativeHeaderPressable + onPressIn={() => { + useAccountStore.getState().setAccountName(lastUsedAccount, firstName, lastName); + router.back(); + }} + > + <Icon papicon size={26}> + <Papicons name="ArrowLeft" /> + </Icon> + </NativeHeaderPressable> + ), + }); + }, [firstName, lastName, lastUsedAccount]);Et supprimez les lignes 170-181 du ScrollView.
172-175: Manque de validation avant la sauvegarde.Les champs
firstNameetlastNamesont sauvegardés sans aucune validation. Cela peut entraîner :
- La sauvegarde de chaînes vides ou contenant uniquement des espaces
- Une expérience utilisateur dégradée si des données invalides sont enregistrées
- Des problèmes d'affichage dans d'autres parties de l'application (initiales vides, etc.)
Ajoutez une validation minimale avant la sauvegarde.
✅ Proposition de validation
<NativeHeaderPressable onPressIn={() => { + const trimmedFirstName = firstName.trim(); + const trimmedLastName = lastName.trim(); + + if (!trimmedFirstName || !trimmedLastName) { + Alert.alert( + t("Error"), + t("Error_FirstName_LastName_Required"), + [{ text: "OK" }] + ); + return; + } + - useAccountStore.getState().setAccountName(lastUsedAccount, firstName, lastName); + useAccountStore.getState().setAccountName(lastUsedAccount, trimmedFirstName, trimmedLastName); router.back(); }} >
65-69: Texte codé en dur non traduit.La chaîne
"Cette fonctionnalité n'est pas encore disponible, mais elle le sera dans une prochaine mise à jour."est codée en dur en français, alors que le reste du fichier utilise des clés de traduction. Cela crée une incohérence et empêche la traduction dans d'autres langues.🌍 Proposition d'internationalisation
Alert.alert( t("Feature_Soon"), - "Cette fonctionnalité n'est pas encore disponible, mais elle le sera dans une prochaine mise à jour.", + t("Feature_Soon_Description"), [{ text: "OK" }] );Ajoutez ensuite la clé
Feature_Soon_Descriptiondans vos fichiers de traduction.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(modals)/profile.tsx
🔇 Additional comments (4)
app/(modals)/profile.tsx (4)
53-53: Excellent !Le changement vers un ratio carré 1:1 est approprié pour les photos de profil et améliore la cohérence visuelle.
76-80: Vérifier le comportement deKeyboardAvoidingView.Bien que la PR mentionne que le comportement était inversé, l'utilisation de
keyboardVerticalOffset={insets.bottom}avecbehavior="position"pourrait ne pas être la solution appropriée pour tous les cas. Sur iOS notamment, cette configuration pourrait créer un décalage excessif.Testez attentivement sur les deux plateformes (iOS et Android) pour vous assurer que :
- Les champs de saisie restent visibles lorsque le clavier apparaît
- Il n'y a pas de décalage excessif vers le haut
- Le comportement est cohérent en mode portrait et paysage
84-84: Bonne approche pour centrer le contenu.L'ajout de
flexGrow: 1etjustifyContent: "center"danscontentContainerStylepermet un centrage vertical élégant du contenu, ce qui améliore l'expérience utilisateur.
89-89: LGTM !L'augmentation de la taille de l'avatar (117 → 150) améliore la visibilité et l'impact visuel de la photo de profil.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/(modals)/profile.tsx (1)
3-4: SupprimeruseHeaderHeightetheightinutilisésLa variable
height(ligne 90) n'est plus utilisée depuis la suppression dupaddingTopconditionnel. L'import et l'appel du hook doivent être nettoyés pour éviter les erreurs de linting (noUnusedLocals).Patch suggéré
-import { useHeaderHeight } from "@react-navigation/elements"; +// import { useHeaderHeight } from "@react-navigation/elements"; @@ - const height = useHeaderHeight(); + // const height = useHeaderHeight();
🧹 Nitpick comments (1)
app/(modals)/profile.tsx (1)
23-54: Le patternbeforeRemoveest supporté par expo-router; optimiser la re-subscription viauseCallback.L'API
useNavigation/beforeRemovefonctionne correctement pour les routes modales. Cependant, incluantfirstNameetlastNamedans les dépendances de l'effet cause une nouvelle souscription à chaque changement de texte. Refactoriser en passantupdateAccountNamecomme dépendance unique et l'envelopper dans unuseCallbackpour réduire le bruit :Exemple de refactoring suggéré
const updateAccountName = useCallback(() => { const trimmedFirstName = firstName.trim(); const trimmedLastName = lastName.trim(); if (!trimmedFirstName || !trimmedLastName) return; useAccountStore.getState().setAccountName(lastUsedAccount, trimmedFirstName, trimmedLastName); }, [firstName, lastName, lastUsedAccount]); useEffect(() => { const unsubscribe = navigation.addListener('beforeRemove', updateAccountName); return unsubscribe; }, [navigation, updateAccountName]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/(modals)/profile.tsxapp/(tabs)/index/atoms/UserProfile.tsx
🔇 Additional comments (3)
app/(modals)/profile.tsx (3)
93-97:KeyboardAvoidingView: validerkeyboardVerticalOffset={insets.bottom}sur iOS/Android.
insets.bottompeut être trop faible (ou incorrect) selon device / modal / header; à tester avec le clavier ouvert sur les champs, surtout sur iOS avec notch et sur Android.
98-106: Centrage + avatar agrandi: RAS (UI cohérente).Le
contentContainerStylecentré (Line 101) + container aligné (Line 104) va dans le sens “modal centré”, et l’avatar à 150 (Line 106) correspond à l’objectif.Also applies to: 103-105
165-183: Vérifier l'existence des clés i18nLabel_FirstNameetLabel_LastNamedans toutes les locales.Bonne suppression des strings hardcodées, mais il est important de s'assurer que ces clés existent dans tous les fichiers de locale. Sans cela, les clés de traduction s'afficheront à la place du texte attendu.
| padding: 20, | ||
| paddingVertical: Platform.OS === "ios" ? 20 : 10, | ||
| backgroundColor: colors.text + (dark ? "15":"08"), | ||
| paddingVertical: Platform.OS === "ios" ? 20 : 5, |
There was a problem hiding this comment.
Je trouve que c'est trop gros sur Android. Après c'est peut être que sur mon tel. C'est pas forcément la bonne PR le changer. Tu peux voir sur les screenshots la différence.
| contentContainerStyle={{ flexGrow: 1, justifyContent: "center" }} | ||
| > | ||
| <View style={{ paddingHorizontal: 50, alignItems: "center", gap: 15, paddingTop: 20 + (Platform.OS === "android" ? height : 0) }}> | ||
| {/* Removed `paddingTop: 20 + (Platform.OS === "android" ? height : 0)` to center */} |
There was a problem hiding this comment.
c'est quoi ce commentaire AI ?? 💀
There was a problem hiding this comment.
C'est pas de l'ia 😭. C'est juste au cas où vous voulez le remettre.
2 participants
Résumé des changements
Changement de la taille du
OnboardingInputen passant leVertical Paddingde10à5.Note
Le padding sur ios est de 20 ça me parait beaucoup.
Modification du fichier
profile.tsxdans(modals)pour avoir une interface centrée. La photo de profil est plus grande. Maintentant on peut sélectionner que les"images"(on pouvait choisir des vidéos). Aussi le ratio était de4:3, je l'ai donc passé à1:1. Fix duKeyboardAvoidingViewqui était inversé donc on voyait pas ce qu'on écrivait.Capture(s) d'écran
Summary by CodeRabbit
Nouvelles Fonctionnalités
Améliorations de l'Interface
Style
✏️ Tip: You can customize this high-level summary in your review settings.