-
Notifications
You must be signed in to change notification settings - Fork 5
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
[HAB-46] Sign in page #39
Conversation
WalkthroughThe recent updates aim to optimize SVG file management and enhance user interface elements in an Expo project. Changes involve improving Metro configuration for efficient SVG support, adding TypeScript declarations for SVG imports, and revamping the sign-in component with new UI elements and functionality to streamline development and enhance user experience. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
[HAB-46] |
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.
Actionable comments posted: 2
src/app/(auth)/signin.tsx
Outdated
const { colorScheme } = useColorScheme(); | ||
|
||
async function signInWithEmail() { | ||
setLoading(true); | ||
const tutorialData = [ | ||
{ pageNum: '1', text: 'Create habits and keep track of completions' }, | ||
{ pageNum: '2', text: 'Add friends and do habits together' }, | ||
{ pageNum: '3', text: 'Help each other reach your goals' }, | ||
]; | ||
const scrollX = useRef<Animated.Value>(new Animated.Value(0)).current; |
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.
The tutorial data setup uses hardcoded text. Consider externalizing this data to a JSON file or a service for easier management and localization.
src/app/(auth)/signin.tsx
Outdated
{Platform.OS === "ios" && ( | ||
<> | ||
<Pressable className="flex flex-row w-11/12 justify-center items-center border-2 dark:border-stone-300 p-2 mt-2 rounded-xl"> | ||
<IconBrandAppleFilled fill={colorScheme === "dark" ? colors.white : colors.black} /> | ||
<Text className="font-semibold text-lg ml-2">Continue with Apple</Text> | ||
</Pressable> | ||
<Pressable className="flex flex-row w-11/12 justify-center items-center border-2 dark:border-stone-300 p-2 mt-2 rounded-xl"> | ||
<Image | ||
className="h-5 w-5 rounded-[20px]" | ||
source={require("../../../assets/images/google.png")} /> | ||
<Text className="font-semibold text-lg ml-2">Continue with Google</Text> | ||
</Pressable> | ||
<Pressable className="mt-2"> | ||
<Text className="font-semibold text-base">Sign up or log in with Email</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "android" && ( | ||
<> | ||
<Pressable className="flex flex-row w-11/12 justify-center items-center border-2 dark:border-stone-300 p-2 mt-2 rounded-xl"> | ||
<Image | ||
className="h-5 w-5 rounded-[20px]" | ||
source={require("../../../assets/images/google.png")} /> | ||
<Text className="font-semibold text-lg ml-2">Continue with Google</Text> | ||
</Pressable> | ||
<Pressable className="flex flex-row w-11/12 justify-center items-center border-2 dark:border-stone-300 p-2 mt-2 rounded-xl"> | ||
<Icon | ||
icon={IconMail} | ||
lightColor={colors.black} | ||
darkColor={colors.white} | ||
strokeWidth={2.2} | ||
/> | ||
<Text className="font-semibold text-lg ml-2">Continue with Email</Text> | ||
</Pressable> | ||
</> | ||
)} |
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.
The platform-specific rendering uses a lot of duplicated code for iOS and Android. Consider abstracting common elements to reduce redundancy and improve maintainability.
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.
Actionable comments posted: 3
src/app/(auth)/signin.tsx
Outdated
slidingIndicatorStyle={{ | ||
backgroundColor: colorScheme === "dark" ? colors.white : colors.black, | ||
}} | ||
/> | ||
|
||
return ( | ||
<View style={styles.container}> | ||
<View style={[styles.verticallySpaced, styles.mt20]}> | ||
<Input | ||
label="Email" | ||
leftIcon={{ type: "font-awesome", name: "envelope" }} | ||
onChangeText={(text) => setEmail(text)} | ||
value={email} | ||
// placeholder="test@gmail.com" | ||
autoCapitalize={"none"} | ||
/> | ||
</View> | ||
<View style={styles.verticallySpaced}> | ||
<Input | ||
label="Password" | ||
leftIcon={{ type: "font-awesome", name: "lock" }} | ||
onChangeText={(text) => setPassword(text)} | ||
value={password} | ||
secureTextEntry={true} | ||
// placeholder="password" | ||
autoCapitalize={"none"} | ||
/> | ||
</View> | ||
<View style={[styles.verticallySpaced, styles.mt20]}> | ||
<Button | ||
title="Sign in" | ||
disabled={loading} | ||
onPress={() => signInWithEmail()} | ||
/> | ||
</View> | ||
<Text className="mb-3 w-2/3 text-center text-xs text-stone-400"> | ||
By continuing, I agree to the {"\n"} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Terms & Conditions | ||
</Text> | ||
</Link>{" "} | ||
and{" "} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Privacy Policy | ||
</Text> | ||
</Link> | ||
</Text> | ||
|
||
{Platform.OS === "ios" && ( | ||
<> | ||
<Pressable className="mt-2 flex w-11/12 flex-row items-center justify-center rounded-xl border-2 p-2 dark:border-stone-300"> | ||
<IconBrandAppleFilled | ||
fill={colorScheme === "dark" ? colors.white : colors.black} | ||
/> | ||
<Text className="ml-2 text-lg font-semibold"> | ||
Continue with Apple | ||
</Text> | ||
</Pressable> | ||
<Pressable className="mt-2 flex w-11/12 flex-row items-center justify-center rounded-xl border-2 p-2 dark:border-stone-300"> | ||
<Image | ||
className="h-5 w-5 rounded-[20px]" | ||
source={require("../../../assets/images/google.png")} | ||
/> | ||
<Text className="ml-2 text-lg font-semibold"> | ||
Continue with Google | ||
</Text> | ||
</Pressable> | ||
<Pressable className="mt-2"> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "android" && ( | ||
<> | ||
<Pressable className="mt-2 flex w-11/12 flex-row items-center justify-center rounded-xl border-2 p-2 dark:border-stone-300"> | ||
<Image | ||
className="h-5 w-5 rounded-[20px]" | ||
source={require("../../../assets/images/google.png")} | ||
/> | ||
<Text className="ml-2 text-lg font-semibold"> | ||
Continue with Google | ||
</Text> | ||
</Pressable> | ||
<Pressable className="mt-2 flex w-11/12 flex-row items-center justify-center rounded-xl border-2 p-2 dark:border-stone-300"> | ||
<Icon | ||
icon={IconMail} | ||
lightColor={colors.black} | ||
darkColor={colors.white} | ||
strokeWidth={2.2} | ||
/> | ||
<Text className="ml-2 text-lg font-semibold"> | ||
Continue with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} |
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.
Tip
Codebase Verification
The verification process has successfully identified that there is indeed duplicated code in the platform-specific checks for iOS and Android within the signin.tsx
file. Both platform checks utilize similar Pressable
and Text
components, which suggests that there might be opportunities for optimization and refactoring to reduce redundancy and improve maintainability.
Recommendations:
- Consider abstracting the common UI components into a separate component or function to reduce duplication and simplify the codebase.
- Ensure that any platform-specific differences are clearly documented and justified to maintain clarity in the code's intent.
Analysis chain
Review the rendering logic and platform-specific code for potential optimizations and ensure consistency in UI components usage.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of UI components across different platforms.
# Test: Check for duplicated code in iOS and Android platform checks.
rg 'Platform.OS === "ios"' src/app/(auth)/signin.tsx -A 20 | rg 'Pressable|Image|Text'
rg 'Platform.OS === "android"' src/app/(auth)/signin.tsx -A 20 | rg 'Pressable|Image|Text'
Length of output: 223
Script:
#!/bin/bash
# Description: Verify consistent usage of UI components across different platforms.
# Test: Check for duplicated code in iOS and Android platform checks.
rg 'Platform.OS === "ios"' 'src/app/(auth)/signin.tsx' -A 20 | rg 'Pressable|Image|Text'
rg 'Platform.OS === "android"' 'src/app/(auth)/signin.tsx' -A 20 | rg 'Pressable|Image|Text'
Length of output: 1238
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.
Actionable comments posted: 1
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.
In addition to all these, there is too much space at the bottom of the screen. Add more space between the header and tutorial picture to make the buttons go at the bottom.
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.
Actionable comments posted: 2
src/app/(auth)/signin.tsx
Outdated
function TutorialItem({ item }: { item: { pageNum: string; text: string } }) { | ||
return ( | ||
<View | ||
className="m-0 items-center justify-center p-0" | ||
style={{ | ||
width: width, | ||
}} | ||
> | ||
<Icon icon={IconPhotoScan} size={270} strokeWidth={0.8} /> | ||
<Text className="px-2 text-center text-3xl font-bold">{item.text}</Text> | ||
</View> | ||
); | ||
} |
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.
The TutorialItem
function is well-implemented for rendering individual tutorial items. Consider using memoization to prevent unnecessary re-renders if the tutorial data does not change often.
- function TutorialItem({ item }: { item: { pageNum: string; text: string } }) {
+ const TutorialItem = React.memo(function({ item }: { item: { pageNum: string; text: string } }) {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
function TutorialItem({ item }: { item: { pageNum: string; text: string } }) { | |
return ( | |
<View | |
className="m-0 items-center justify-center p-0" | |
style={{ | |
width: width, | |
}} | |
> | |
<Icon icon={IconPhotoScan} size={270} strokeWidth={0.8} /> | |
<Text className="px-2 text-center text-3xl font-bold">{item.text}</Text> | |
</View> | |
); | |
} | |
const TutorialItem = React.memo(function({ item }: { item: { pageNum: string; text: string } }) { | |
return ( | |
<View | |
className="m-0 items-center justify-center p-0" | |
style={{ | |
width: width, | |
}} | |
> | |
<Icon icon={IconPhotoScan} size={270} strokeWidth={0.8} /> | |
<Text className="px-2 text-center text-3xl font-bold">{item.text}</Text> | |
</View> | |
); | |
} |
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.
Actionable comments posted: 6
src/app/(auth)/signin.tsx
Outdated
@@ -26,75 +43,161 @@ import { resetNavigationStack } from "@/src/lib/resetNavigationStack"; | |||
// const app = initializeApp(firebaseConfig); | |||
// const analytics = getAnalytics(app); | |||
|
|||
|
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.
Ensure proper cleanup of commented-out Firebase initialization code.
Consider removing the commented-out Firebase initialization code if it's no longer needed, or properly integrate it if it's supposed to be part of the application's functionality.
src/app/(auth)/signin.tsx
Outdated
const tutorialData = [ | ||
{ pageNum: "1", text: "Create habits and keep track of completions" }, | ||
{ pageNum: "2", text: "Add friends and do habits together" }, | ||
{ pageNum: "3", text: "Help each other reach your goals" }, | ||
]; |
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.
Externalize tutorial data to a JSON file or a service for easier management and localization.
The tutorial data is hardcoded within the component. Consider externalizing this data to a JSON file or a service to facilitate easier updates and localization. Would you like me to help with this implementation?
src/app/(auth)/signin.tsx
Outdated
source={require("../../../assets/images/Logo.svg")} | ||
/> */} | ||
<Logo height={24} width={24} /> | ||
<Text className="ml-2 text-2xl font-bold">Habit Together</Text> |
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.
Correct the application name to match the brand guidelines.
The text "Habit Together" should be corrected to "Habits Together" as per the previous comments and brand guidelines.
- <Text className="ml-2 text-2xl font-bold">Habit Together</Text>
+ <Text className="ml-2 text-2xl font-bold">Habits Together</Text>
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
<Text className="ml-2 text-2xl font-bold">Habit Together</Text> | |
<Text className="ml-2 text-2xl font-bold">Habits Together</Text> |
src/app/(auth)/signin.tsx
Outdated
<Pressable className="rounded-2xl bg-stone-200 px-4 py-1 dark:border-2 dark:border-stone-300 dark:bg-stone-800"> | ||
<Text className="text-base font-semibold">Skip</Text> | ||
</Pressable> |
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.
Adjust the dark mode styling for the 'Skip' button to match the design specifications.
The 'Skip' button should not have a border in dark mode, and should only have a grey fill according to the design specifications.
- <Pressable className="rounded-2xl bg-stone-200 px-4 py-1 dark:border-2 dark:border-stone-300 dark:bg-stone-800">
+ <Pressable className="rounded-2xl bg-stone-200 px-4 py-1 dark:bg-stone-800">
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
<Pressable className="rounded-2xl bg-stone-200 px-4 py-1 dark:border-2 dark:border-stone-300 dark:bg-stone-800"> | |
<Text className="text-base font-semibold">Skip</Text> | |
</Pressable> | |
<Pressable className="rounded-2xl bg-stone-200 px-4 py-1 dark:bg-stone-800"> | |
<Text className="text-base font-semibold">Skip</Text> | |
</Pressable> |
src/app/(auth)/signin.tsx
Outdated
{Platform.OS === "ios" && ( | ||
<> | ||
<AppleButton /> | ||
<GoogleButton /> | ||
<Pressable className="mt-2"> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "android" && ( | ||
<> | ||
<GoogleButton /> | ||
<EmailButton /> | ||
</> | ||
)} |
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.
Reduce redundancy by abstracting common elements in platform-specific rendering paths.
There is duplicated code in the platform-specific rendering for iOS and Android. Consider creating a single SignInButton
component that takes platform-specific properties to reduce duplication and improve maintainability.
- {Platform.OS === "ios" && (
- <>
- <AppleButton />
- <GoogleButton />
- <Pressable className="mt-2">
- <Text className="text-base font-semibold">
- Sign up or log in with Email
- </Text>
- </Pressable>
- </>
- )}
- {Platform.OS === "android" && (
- <>
- <GoogleButton />
- <EmailButton />
- </>
- )}
+ <SignInButtons platform={Platform.OS} />
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
{Platform.OS === "ios" && ( | |
<> | |
<AppleButton /> | |
<GoogleButton /> | |
<Pressable className="mt-2"> | |
<Text className="text-base font-semibold"> | |
Sign up or log in with Email | |
</Text> | |
</Pressable> | |
</> | |
)} | |
{Platform.OS === "android" && ( | |
<> | |
<GoogleButton /> | |
<EmailButton /> | |
</> | |
)} | |
<SignInButtons platform={Platform.OS} /> |
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.
Actionable comments posted: 2
src/app/(auth)/signin.tsx
Outdated
scrollX={scrollX} | ||
dotSize={10} | ||
containerStyle={{ position: "relative" }} | ||
dotStyle={{ | ||
backgroundColor: colorScheme === "dark" ? colors.white : colors.black, | ||
}} | ||
marginHorizontal={10} | ||
slidingIndicatorStyle={{ | ||
backgroundColor: colorScheme === "dark" ? colors.white : colors.black, | ||
}} | ||
/> | ||
|
||
<Text className="mb-3 w-2/3 text-center text-xs text-stone-400"> | ||
By continuing, I agree to the {"\n"} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Terms & Conditions | ||
</Text> | ||
</Link>{" "} | ||
and{" "} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Privacy Policy | ||
</Text> | ||
</Link> | ||
</Text> | ||
|
||
<View className="w-full flex justify-center items-center px-4"> | ||
{Platform.OS === "ios" && ( | ||
<> | ||
<AppleButton /> | ||
<GoogleButton /> | ||
<Pressable className="mt-2"> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "android" && ( | ||
<> | ||
<GoogleButton /> | ||
<EmailButton /> | ||
</> | ||
)} | ||
</View> | ||
</View> | ||
); | ||
} | ||
|
||
const styles = StyleSheet.create({ | ||
container: { | ||
marginTop: 40, | ||
padding: 12, | ||
}, | ||
verticallySpaced: { | ||
paddingTop: 4, | ||
paddingBottom: 4, | ||
alignSelf: "stretch", | ||
}, | ||
mt20: { | ||
marginTop: 20, | ||
}, | ||
}); | ||
function GoogleButton() { | ||
return ( | ||
<Pressable | ||
className="h-14 mt-2 flex w-full flex-row items-center justify-center rounded-xl border-2 p-2 dark:bg-white" | ||
onPress={signInWithGoogle} | ||
> | ||
<Image className="h-5 w-5 rounded-[20px]" source={require("../../../assets/images/google.png")} /> | ||
<Text className="ml-2 text-lg font-semibold text-black">Continue with Google</Text> | ||
</Pressable> | ||
) | ||
} | ||
|
||
function AppleButton() { | ||
return ( | ||
<Pressable | ||
className="h-14 mt-2 flex w-full flex-row items-center justify-center rounded-xl border-2 p-2 dark:bg-white" | ||
onPress={signInWithApple} | ||
> | ||
<Image className="h-5 w-5 rounded-[20px]" source={require("../../../assets/images/apple.png")} /> | ||
<Text className="ml-2 text-lg font-semibold text-black">Continue with Apple</Text> | ||
</Pressable> | ||
) | ||
} | ||
|
||
function EmailButton() { | ||
return ( | ||
<Pressable | ||
className="h-14 mt-2 flex w-full flex-row items-center justify-center rounded-xl border-2 p-2 dark:bg-white" | ||
onPress={signInWithEmail} | ||
> | ||
<Icon icon={IconMail} lightColor={colors.black} darkColor={colors.black} strokeWidth={2.2} /> | ||
<Text className="ml-2 text-lg font-semibold text-black">Continue with Email</Text> | ||
</Pressable> | ||
) | ||
} | ||
} |
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.
Review the overall structure and logic of the Signin
function. Ensure that the function's complexity is manageable and that it adheres to best practices for readability and maintainability.
Consider breaking down the Signin
function into smaller, more manageable components or hooks to improve readability and maintainability.
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.
Actionable comments posted: 3
src/app/(auth)/signin.tsx
Outdated
value={email} | ||
// placeholder="test@gmail.com" | ||
autoCapitalize={"none"} | ||
/> | ||
<View className="flex-1 items-center pt-14"> | ||
<View className="w-full px-4 flex-row items-center justify-between pb-5"> | ||
<View className="flex-row items-center"> | ||
{/* <Image | ||
className="h-6 w-6 rounded-[20px]" | ||
source={require("../../../assets/images/Logo.svg")} | ||
/> */} | ||
<Logo height={24} width={24} /> | ||
<Text className="ml-2 text-2xl font-bold">Habits Together</Text> | ||
</View> | ||
<Pressable className="rounded-2xl bg-stone-200 dark:bg-stone-700 px-4 py-1" | ||
onPress={continueAsGuest}> | ||
<Text className="text-base font-semibold">Skip</Text> | ||
</Pressable> | ||
</View> | ||
<View style={styles.verticallySpaced}> | ||
<Input | ||
label="Password" | ||
leftIcon={{ type: "font-awesome", name: "lock" }} | ||
onChangeText={(text) => setPassword(text)} | ||
value={password} | ||
secureTextEntry={true} | ||
// placeholder="password" | ||
autoCapitalize={"none"} | ||
|
||
<View className="flex-1 w-full flex justify-center items-center pb-3"> | ||
<FlatList | ||
data={tutorialData} | ||
renderItem={({ item }) => TutorialItem({ item })} | ||
horizontal | ||
pagingEnabled | ||
showsHorizontalScrollIndicator={false} | ||
keyExtractor={(item) => item.pageNum} | ||
onScroll={Animated.event( | ||
[{ nativeEvent: { contentOffset: { x: scrollX } } }], | ||
{ | ||
useNativeDriver: false, | ||
}, | ||
)} | ||
className="mb-14 grow-0" | ||
/> | ||
</View> | ||
<View style={[styles.verticallySpaced, styles.mt20]}> | ||
<Button | ||
title="Sign in" | ||
disabled={loading} | ||
onPress={() => signInWithEmail()} | ||
|
||
<SlidingDot | ||
data={tutorialData} | ||
scrollX={scrollX} | ||
dotSize={10} | ||
containerStyle={{ position: "relative" }} | ||
dotStyle={{ | ||
backgroundColor: colorScheme === "dark" ? colors.white : colors.black, | ||
}} | ||
marginHorizontal={10} | ||
slidingIndicatorStyle={{ | ||
backgroundColor: colorScheme === "dark" ? colors.white : colors.black, | ||
}} | ||
/> | ||
|
||
<Text className="mb-3 w-2/3 text-center text-xs text-stone-400"> | ||
By continuing, I agree to the {"\n"} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Terms & Conditions | ||
</Text> | ||
</Link>{" "} | ||
and{" "} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Privacy Policy | ||
</Text> | ||
</Link> | ||
</Text> | ||
|
||
<View className="w-full flex justify-center items-center px-4"> | ||
{Platform.OS === "android" && ( | ||
<> | ||
<AppleButton /> | ||
<GoogleButton /> | ||
<Pressable className="mt-2"> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "ios" && ( | ||
<> | ||
<GoogleButton /> | ||
<EmailButton /> | ||
</> | ||
)} | ||
</View> |
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.
Consider refactoring the Signin
function to improve readability and maintainability.
The Signin
function is quite large and handles multiple responsibilities. Consider breaking it down into smaller components or hooks. This will not only improve readability but also make the code easier to maintain and test.
src/app/(auth)/signin.tsx
Outdated
{Platform.OS === "android" && ( | ||
<> | ||
<AppleButton /> | ||
<GoogleButton /> | ||
<Pressable className="mt-2"> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "ios" && ( | ||
<> | ||
<GoogleButton /> | ||
<EmailButton /> | ||
</> | ||
)} |
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.
Reduce redundancy by abstracting common elements in platform-specific rendering paths.
There is duplicated code in the platform-specific rendering for iOS and Android. Consider creating a single SignInButton
component that takes platform-specific properties to reduce duplication and improve maintainability.
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.
Actionable comments posted: 4
src/app/(auth)/signin.tsx
Outdated
// placeholder="test@gmail.com" | ||
autoCapitalize={"none"} | ||
/> | ||
<View className="flex-1 items-center pt-14"> | ||
<View className="w-full px-4 flex-row items-center justify-between pb-5"> | ||
<View className="flex-row items-center"> | ||
{/* <Image | ||
className="h-6 w-6 rounded-[20px]" | ||
source={require("../../../assets/images/Logo.svg")} | ||
/> */} | ||
<Logo height={24} width={24} /> | ||
<Text className="ml-2 text-2xl font-bold">Habits Together</Text> | ||
</View> | ||
<Pressable className="rounded-2xl bg-stone-200 dark:bg-stone-700 px-4 py-1" | ||
onPress={continueAsGuest}> | ||
<Text className="text-base font-semibold">Skip</Text> | ||
</Pressable> | ||
</View> | ||
<View style={styles.verticallySpaced}> | ||
<Input | ||
label="Password" | ||
leftIcon={{ type: "font-awesome", name: "lock" }} | ||
onChangeText={(text) => setPassword(text)} | ||
value={password} | ||
secureTextEntry={true} | ||
// placeholder="password" | ||
autoCapitalize={"none"} | ||
|
||
<View className="flex-1 w-full flex justify-center items-center pb-3"> | ||
<FlatList | ||
data={tutorialData} | ||
renderItem={({ item }) => TutorialItem({ item })} | ||
horizontal | ||
pagingEnabled | ||
showsHorizontalScrollIndicator={false} | ||
keyExtractor={(item) => item.pageNum} | ||
onScroll={Animated.event( | ||
[{ nativeEvent: { contentOffset: { x: scrollX } } }], | ||
{ | ||
useNativeDriver: false, | ||
}, | ||
)} | ||
className="mb-14 grow-0" | ||
/> | ||
</View> | ||
<View style={[styles.verticallySpaced, styles.mt20]}> | ||
<Button | ||
title="Sign in" | ||
disabled={loading} | ||
onPress={() => signInWithEmail()} | ||
|
||
<SlidingDot | ||
data={tutorialData} | ||
scrollX={scrollX} | ||
dotSize={10} | ||
containerStyle={{ position: "relative" }} | ||
dotStyle={{ | ||
backgroundColor: colorScheme === "dark" ? colors.white : colors.black, | ||
}} | ||
marginHorizontal={10} | ||
slidingIndicatorStyle={{ | ||
backgroundColor: colorScheme === "dark" ? colors.white : colors.black, | ||
}} | ||
/> | ||
|
||
<Text className="mb-3 w-2/3 text-center text-xs text-stone-400"> | ||
By continuing, I agree to the {"\n"} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Terms & Conditions | ||
</Text> | ||
</Link>{" "} | ||
and{" "} | ||
<Link href="/"> | ||
<Text className="text-xs text-stone-400 underline"> | ||
Privacy Policy | ||
</Text> | ||
</Link> | ||
</Text> | ||
|
||
<View className="w-full flex justify-center items-center px-4"> | ||
{Platform.OS === "ios" && ( | ||
<> | ||
<AppleButton /> | ||
<GoogleButton /> | ||
<Pressable className="mt-2" | ||
onPress={signInWithEmail}> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "android" && ( | ||
<> | ||
<GoogleButton /> | ||
<EmailButton /> | ||
</> | ||
)} | ||
</View> |
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.
Consider refactoring the Signin
function to improve readability and maintainability. The function is quite large and handles multiple responsibilities. Breaking it down into smaller components or hooks will not only improve readability but also make the code easier to maintain and test.
src/app/(auth)/signin.tsx
Outdated
{Platform.OS === "ios" && ( | ||
<> | ||
<AppleButton /> | ||
<GoogleButton /> | ||
<Pressable className="mt-2" | ||
onPress={signInWithEmail}> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "android" && ( | ||
<> | ||
<GoogleButton /> | ||
<EmailButton /> | ||
</> | ||
)} |
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.
Reduce redundancy by abstracting common elements in platform-specific rendering paths. Consider creating a single SignInButton
component that takes platform-specific properties to reduce duplication and improve maintainability.
- {Platform.OS === "ios" && (
- <>
- <AppleButton />
- <GoogleButton />
- <Pressable className="mt-2">
- <Text className="text-base font-semibold">
- Sign up or log in with Email
- </Text>
- </Pressable>
- </>
- )}
- {Platform.OS === "android" && (
- <>
- <GoogleButton />
- <EmailButton />
- </>
- )}
+ <SignInButtons platform={Platform.OS} />
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
{Platform.OS === "ios" && ( | |
<> | |
<AppleButton /> | |
<GoogleButton /> | |
<Pressable className="mt-2" | |
onPress={signInWithEmail}> | |
<Text className="text-base font-semibold"> | |
Sign up or log in with Email | |
</Text> | |
</Pressable> | |
</> | |
)} | |
{Platform.OS === "android" && ( | |
<> | |
<GoogleButton /> | |
<EmailButton /> | |
</> | |
)} | |
<SignInButtons platform={Platform.OS} /> |
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.
Actionable comments posted: 5
Out of diff range and nitpick comments (2)
src/app/(auth)/signin.tsx (2)
177-191
: Ensure that the Google sign-in button is using a high-resolution icon as per the design specifications.
197-207
: Ensure that the Apple sign-in button is using a high-resolution icon as per the design specifications.
import Icon from "@/src/components/Icon"; | ||
import { SlidingDot } from "react-native-animated-pagination-dots"; | ||
import { useRef, useState } from "react"; | ||
import Logo from "@/assets/images/Logo.svg"; |
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.
Consider removing the commented-out Firebase initialization code if it's no longer needed.
const { colorScheme } = useColorScheme(); | ||
const { width } = Dimensions.get("window"); | ||
const [email, setEmail] = useState("test@gmail.com"); | ||
const [password, setPassword] = useState("password"); | ||
const [loading, setLoading] = useState(false); | ||
|
||
async function signInWithGoogle() { | ||
resetNavigationStack("/"); | ||
} | ||
async function signInWithApple() { | ||
resetNavigationStack("/"); | ||
} | ||
async function signInWithEmail() { | ||
setLoading(true); | ||
|
||
// see if the account exists | ||
|
||
setLoading(false); | ||
resetNavigationStack("/"); | ||
} | ||
async function continueAsGuest() { | ||
resetNavigationStack("/"); | ||
} |
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.
Tip
Codebase Verification
The verification process has confirmed that the sign-in functions (signInWithGoogle
, signInWithApple
, signInWithEmail
, continueAsGuest
) are indeed present in the src/app/(auth)/signin.tsx
file. However, the implementation of these functions, as shown in the provided code snippet, only includes calls to resetNavigationStack("/")
, which suggests that the actual authentication logic might not be implemented within these functions.
Given this, it seems that the original review comment is accurate in pointing out that the current implementation might not be sufficient for actual authentication processes. The functions are present, but they do not perform any authentication-related tasks beyond resetting the navigation stack.
Action Required:
- Review and possibly extend the implementation of the sign-in functions to include actual authentication logic.
Analysis chain
Ensure that the sign-in functions (signInWithGoogle
, signInWithApple
, signInWithEmail
, continueAsGuest
) are properly implemented. Currently, they only reset the navigation stack, which might not be sufficient for actual authentication processes.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the sign-in functions are properly implemented.
# Test: Search for the sign-in function implementations.
rg 'signInWithGoogle|signInWithApple|signInWithEmail|continueAsGuest' src/app/(auth)/signin.tsx
Length of output: 232
Script:
#!/bin/bash
# Description: Verify that the sign-in functions are properly implemented.
# Test: Search for the sign-in function implementations.
rg 'signInWithGoogle|signInWithApple|signInWithEmail|continueAsGuest' src/app/auth/signin.tsx
Length of output: 159
Script:
#!/bin/bash
# Description: Verify that the sign-in functions are properly implemented.
# Test: Search for the sign-in function implementations.
rg 'signInWithGoogle|signInWithApple|signInWithEmail|continueAsGuest' --glob '*signin.tsx'
Length of output: 683
function TutorialItem({ item }: { item: { pageNum: string; text: string } }) { | ||
return ( | ||
<View | ||
className="m-0 items-center justify-center p-0" | ||
style={{ | ||
width: width, | ||
}} | ||
> | ||
<Icon icon={IconPhotoScan} size={270} strokeWidth={0.8} /> | ||
<Text className="px-2 text-center text-3xl font-bold">{item.text}</Text> | ||
</View> | ||
); |
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.
Consider using memoization for the TutorialItem
function to prevent unnecessary re-renders if the tutorial data does not change often.
- function TutorialItem({ item }: { item: { pageNum: string; text: string } }) {
+ const TutorialItem = React.memo(function({ item }: { item: { pageNum: string; text: string } }) {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
function TutorialItem({ item }: { item: { pageNum: string; text: string } }) { | |
return ( | |
<View | |
className="m-0 items-center justify-center p-0" | |
style={{ | |
width: width, | |
}} | |
> | |
<Icon icon={IconPhotoScan} size={270} strokeWidth={0.8} /> | |
<Text className="px-2 text-center text-3xl font-bold">{item.text}</Text> | |
</View> | |
); | |
const TutorialItem = React.memo(function({ item }: { item: { pageNum: string; text: string } }) { | |
return ( | |
<View | |
className="m-0 items-center justify-center p-0" | |
style={{ | |
width: width, | |
}} | |
> | |
<Icon icon={IconPhotoScan} size={270} strokeWidth={0.8} /> | |
<Text className="px-2 text-center text-3xl font-bold">{item.text}</Text> | |
</View> | |
); |
const tutorialData = [ | ||
{ pageNum: "1", text: "Create habits and keep track of completions" }, | ||
{ pageNum: "2", text: "Add friends and do habits together" }, | ||
{ pageNum: "3", text: "Put interactive widgets on your home screen" }, | ||
]; |
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.
Externalize tutorial data to a JSON file or a service for easier management and localization.
Would you like me to help with this implementation?
<View className="flex w-full items-center justify-center px-4"> | ||
{Platform.OS === "ios" && ( | ||
<> | ||
<AppleButton /> | ||
<GoogleButton /> | ||
<Pressable className="mt-2" onPress={signInWithEmail}> | ||
<Text className="text-base font-semibold"> | ||
Sign up or log in with Email | ||
</Text> | ||
</Pressable> | ||
</> | ||
)} | ||
{Platform.OS === "android" && ( | ||
<> | ||
<GoogleButton /> | ||
<EmailButton /> | ||
</> | ||
)} | ||
</View> |
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.
Reduce redundancy by abstracting common elements in platform-specific rendering paths. Consider creating a single SignInButton
component that takes platform-specific properties to reduce duplication and improve maintainability.
- {Platform.OS === "ios" && (
- <>
- <AppleButton />
- <GoogleButton />
- <Pressable className="mt-2">
- <Text className="text-base font-semibold">
- Sign up or log in with Email
- </Text>
- </Pressable>
- </>
- )}
- {Platform.OS === "android" && (
- <>
- <GoogleButton />
- <EmailButton />
- </>
- )}
+ <SignInButtons platform={Platform.OS} />
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
<View className="flex w-full items-center justify-center px-4"> | |
{Platform.OS === "ios" && ( | |
<> | |
<AppleButton /> | |
<GoogleButton /> | |
<Pressable className="mt-2" onPress={signInWithEmail}> | |
<Text className="text-base font-semibold"> | |
Sign up or log in with Email | |
</Text> | |
</Pressable> | |
</> | |
)} | |
{Platform.OS === "android" && ( | |
<> | |
<GoogleButton /> | |
<EmailButton /> | |
</> | |
)} | |
</View> | |
<View className="flex w-full items-center justify-center px-4"> | |
<SignInButtons platform={Platform.OS} /> | |
</View> |
[HAB-46] Sign in page