Conversation
Hantex9
left a comment
Hantex9
left a comment
There was a problem hiding this comment.
Good job, Pawel!
Just a few small improvements and considerations, you're on the right track
There was a problem hiding this comment.
Be careful, this file should never be modified. Make sure your environment is clean before starting the iOS build.
| export const ProfileFields = () => { | ||
| const toyProfilePot = useIOSelector(toyProfileSelector); | ||
|
|
||
| const profile = pot.isSome(toyProfilePot) ? toyProfilePot.value : undefined; |
There was a problem hiding this comment.
You can consider using the .toUndefined() method of the pot, which automatically returns the pot value if it exists, or undefined otherwise.
| const profile = pot.isSome(toyProfilePot) ? toyProfilePot.value : undefined; | |
| const profile = pot.toUndefined(toyProfilePot); |
| if (!profile) { | ||
| return <LoadingSpinner />; | ||
| } |
There was a problem hiding this comment.
What happens if there's an error in the API request and we don't have the profile?
In this case, the app shows an infinite loading spinner without providing the user with proper feedback.
Consider using the pot.fold method to map each case to the correct output screen.
For example:
- If the pot is in an error state, show an error screen (using
OperationResultScreenContent). - If it's loading, display a loading spinner.
- And so on for the other possible states.
|
|
||
| return ( | ||
| <LoadingSpinnerOverlay | ||
| isLoading={pot.isNone(toyProfilePot) || pot.isLoading(toyProfilePot)} |
| navigation: IOStackNavigationProp< | ||
| ToyProfileParamsList, | ||
| "PROFILE_CONFIRM_DELETE" | ||
| >; | ||
| }; | ||
|
|
||
| export const ProfileConfirmDeleteScreen = ({ navigation }: Props) => { | ||
| const dispatch = useDispatch(); |
There was a problem hiding this comment.
Instead of using the navigation component prop, consider using the useIONavigation hook directly.
This way, you don’t need to add any extra boilerplate about the navigation typing, you can simply do inside the component:
const navigation = useIONavigation();| const ProfileHomeScreen = ({ navigation }: Props) => { | ||
| const dispatch = useDispatch(); | ||
|
|
| const delStatus = pot.isSome(deletePot) ? deletePot.value?.status : undefined; | ||
| const delDisabled = | ||
| pot.isLoading(deletePot) || pot.isNone(deletePot) || !!delStatus; | ||
| const delValue = |
There was a problem hiding this comment.
Consider renaming the variables to make them more self-explanatory.
There was a problem hiding this comment.
In this case since the file contains both the handlers and the watcher, consider renaming the file to index.ts
There was a problem hiding this comment.
I think, just to follow our conventions, it will be better to separate this into two different files:
- in index we can keep the
watchToyProfileSaga - in another file the
loadToyProfileSagaand I think it will be better renamed ithandleLoadToyProfileSaga
This reverts commit e5648f0.
soixdev91
left a comment
soixdev91
left a comment
There was a problem hiding this comment.
updated with requested changes
| export const ProfileFields = () => { | ||
| const toyProfilePot = useIOSelector(toyProfileSelector); | ||
|
|
||
| const profile = pot.isSome(toyProfilePot) ? toyProfilePot.value : undefined; |
| if (!profile) { | ||
| return <LoadingSpinner />; | ||
| } |
|
|
||
| return ( | ||
| <LoadingSpinnerOverlay | ||
| isLoading={pot.isNone(toyProfilePot) || pot.isLoading(toyProfilePot)} |
| navigation: IOStackNavigationProp< | ||
| ToyProfileParamsList, | ||
| "PROFILE_CONFIRM_DELETE" | ||
| >; | ||
| }; | ||
|
|
||
| export const ProfileConfirmDeleteScreen = ({ navigation }: Props) => { | ||
| const dispatch = useDispatch(); |
3 participants
No description provided.