Remove the old avatar JSON file#315
Conversation
Test Results24 tests 24 ✅ 2s ⏱️ Results for commit 196bd4a. ♻️ This comment has been updated with latest results. |
|
d881d1e to
66aa6d3
Compare
| export function useAvatarListQuery() { | ||
| const avatarListQuery = useQuery<AvatarListDTO>({ | ||
| queryKey: avatarKeys.all, | ||
| queryFn: () => fetchAvatarList(), |
There was a problem hiding this comment.
I think the data you fetched from Azure is consistent, so usually you don't need to fetch again. You can add
staleTime: Infinity,
gcTime: Infinity,
| const isSelected = avatar.url === selectedAvatarUrl; | ||
| {isAvatarListError ? ( | ||
| <Text className="w-full text-center text-base font-baloo text-secondary"> | ||
| Failed to load avatars. |
There was a problem hiding this comment.
You should not hard code the string here add Chinese language support.
| if (avatars.some((avatar) => avatar.url === pictureUrl)) { | ||
| setSelectedAvatarUrl(pictureUrl); | ||
| } | ||
| }, [avatars, userProfile?.pictureUrl]); |
There was a problem hiding this comment.
What's this useEffect doing here? If it's just to fetch the default value for avatar URL, I think you can just add it inside of useState instead of using another useEffect here.
| const { userProfile } = useUserProfile(); | ||
| const { avatars, isAvatarListLoading, isAvatarListError } = useAvatarListQuery(); | ||
|
|
||
| const [selectedAvatarUrl, setSelectedAvatarUrl] = useState<string | null>(null); |
There was a problem hiding this comment.
I think we already have the avatar URL set in user profile, so you don't need to add another local state of avatar URL here.If you need the URL, just read it from the user profile.
| const handleAvatarSelect = async (avatar: AvatarDTO) => { | ||
| if (isUserUpdating) return; | ||
|
|
||
| const previousAvatarUrl = selectedAvatarUrl; |
There was a problem hiding this comment.
I also think it's not very useful to set a previous avatar URL here because you already use updateStateAvatar. Since you're using the useMutation function, if it fails on error, it will roll back itself. The user profile won't be updated, so you still can read the old cache value.
| isOnBoarded: userProfile?.isOnBoarded ?? false, | ||
| }); | ||
| } catch { | ||
| setSelectedAvatarUrl(previousAvatarUrl); |
There was a problem hiding this comment.
import { Pressable, Text, View } from "react-native";
import { ReturnButton } from "@/shared/components/return-button";
import { useUserProfileMutation } from "@/feature/settings/hooks/useUserProfileMutation";
import { useUserProfile } from "@/shared/hooks/useUserProfile";
import { AvatarDTO } from "@/feature/settings/modals/avatar-dto";
import { useAvatarListQuery } from "@/feature/settings/hooks/useAvatarListQuery";
import LoadingScreen from "@/shared/components/loading-screen";
import { SafeAreaView } from "react-native-safe-area-context";
import { Image } from "expo-image";
export default function SettingsAvatarScreen() {
const { userProfile } = useUserProfile();
const { avatars, isAvatarListLoading, isAvatarListError } = useAvatarListQuery();
const { updateUserProfile, isUserUpdating } = useUserProfileMutation();
if (isAvatarListLoading) {
return ;
}
const handleAvatarSelect = async (avatar: AvatarDTO) => {
if (isUserUpdating) return;
try {
await updateUserProfile({
displayName: userProfile?.displayName ?? "",
pictureUrl: avatar.url,
isOnBoarded: userProfile?.isOnBoarded ?? false,
});
} catch {
console.log("Failed to update avatar.");
}
};
return (
Avatar
<View className="flex-row flex-wrap justify-between m-6">
{isAvatarListError ? (
<Text className="w-full text-center text-base font-baloo text-secondary">
Failed to load avatars.
</Text>
) : (
avatars.map((avatar) => {
const isSelected = avatar.url === userProfile?.pictureUrl;
return (
<Pressable
key={avatar.url}
onPress={() => handleAvatarSelect(avatar)}
className={`mb-6 items-center ${isUserUpdating ? "opacity-70" : ""}`}
disabled={isUserUpdating}
>
<View
className="rounded-full border-4"
style={{
borderColor: isSelected ? "#8B86B3" : "transparent",
}}
>
<Image
source={{ uri: avatar.url }}
style={{ width: 80, height: 80, borderRadius: 48 }}
contentFit="cover"
/>
</View>
</Pressable>
);
})
)}
</View>
</SafeAreaView>
);
}
I generated this part from claude to see if this logic will make the code look better. It feels like everything's still working in this.
| ); | ||
| } | ||
|
|
||
| function isAvatarListDTO(value: unknown): value is AvatarListDTO { |
There was a problem hiding this comment.
I'm not sure whether we need a very complex type check here, because we maintain this JSON file ourselves, so usually it shouldn't have any type error.
If you want to do type validation, please use Zod instead of a manual validate function.
export const AvatarDTOSchema = z.object({
id: z.string(),
label: z.string(),
url: z.string(),
});
const AvatarListDTOSchema = z.object({
version: z.number(),
avatars: z.array(AvatarDTOSchema),
});
66aa6d3 to
ae3a883
Compare
ae3a883 to
196bd4a
Compare
| import { AvatarDTO } from "@/feature/settings/modals/avatar-dto"; | ||
| import { LOCAL_AVATARS } from "@/feature/settings/constants/local-avatar-catalog"; | ||
|
|
||
| export function useAvatarListQuery() { |
There was a problem hiding this comment.
i don‘t think you need this. if you store avatr locally, there won't be loading and error state
| : avatarDataDevelopment; | ||
| const avatars = avatarData.avatars; | ||
| const { userProfile } = useUserProfile(); | ||
| const { avatars, isAvatarListLoading, isAvatarListError } = useAvatarListQuery(); |
There was a problem hiding this comment.
If you don't use query, this can also be removed.
|
|
||
| const { updateUserProfile, isUserUpdating } = useUserProfileMutation(); | ||
|
|
||
| if (isAvatarListLoading) { |
There was a problem hiding this comment.
I don't feel like there will be an avatar loading state. You can delete this.
| const { userProfile } = useUserProfile(); | ||
| const { avatars, isAvatarListLoading, isAvatarListError } = useAvatarListQuery(); | ||
|
|
||
| const [selectedAvatarId, setSelectedAvatarId] = useState<string | null>(null); |
There was a problem hiding this comment.
Also, there is no need to hold a local state for the selected avatar ID. When a user clicks a new avatar, just upload it and always use userProfile.avatar as the single source of truth.
| <View className="flex-row flex-wrap justify-between m-6"> | ||
| {avatars.map((avatar) => { | ||
| const isSelected = avatar.url === selectedAvatarUrl; | ||
| {isAvatarListError ? ( |
There was a problem hiding this comment.
I don't feel like there will be an error state. I can't delete this.
| : PNGIMAGES.blotzIcon; | ||
| const pictureValue = userProfile?.pictureUrl; | ||
| const SelectedAvatarComponent = getLocalAvatarComponent(pictureValue); | ||
| const avatarSource = isRemoteAvatarUrl(pictureValue) ? { uri: pictureValue } : PNGIMAGES.blotzIcon; |
There was a problem hiding this comment.
const SelectedAvatarComponent = getLocalAvatarComponent(pictureValue);
const avatarSource = pictureValue && !SelectedAvatarComponent
? { uri: pictureValue }
: PNGIMAGES.blotzIcon;
I feel like this works the same, and you can remove the function isRemoteAvatarUrl
| /> | ||
| )} | ||
| <Pressable | ||
| onPress={handleProfileEdit} |
There was a problem hiding this comment.
Also, if you didn't add any new package, you shouldn't have changes in package-lock.json.
You can copy the package-lock.json from main to cover your local package-lock.json to avoid changes.
No description provided.