-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adds user authentication workflow #3
Conversation
const authContext = useContext(AuthContext); | ||
const [username, setUsername] = useState(""); | ||
const [password, setPassword] = useState(""); | ||
const [username, setUsername] = useState(); |
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.
@ndedhia1210 Updated this line per your comment on the previous PR.
396bab7
to
b6e14df
Compare
App.js
Outdated
import { NavigationContainer } from "@react-navigation/native"; | ||
import * as SplashScreen from "expo-splash-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.
I liked this.
App.js
Outdated
} finally { | ||
setIsReady(true); | ||
} |
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.
Do we need Finally? Because incase of error we should display error screen and having boolean for ready will not distinguish whether it is succeeded or failed?
How about we initialize loadingState which can have values like [Succeeded, Failed, Loading, Idle]
So basically
// Some other file
export enum AsyncStatus {
Idle = 'Idle',
Loading = 'Loading',
Succeeded = 'Succeeded',
Failed = 'Failed',
}
// This file
const [appStatus, setAppStatus] = useState(Async.Idle);
....
try {
...
setAppStatus(Async.Succeeded)
} catch(error) {
setAppStatus(Async.Failed)
}
...
// Just demonstrating what we should render
switch(appStatus) {
case Async.Idle: return null;
case Async.Failed: return <ErrorComponent />;
case Async.Succeeded: return <ActualComponent />;
case Async.Loading: return <SplashScreen />;
}
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.
Loved this idea.. Updated
app/api/client.js
Outdated
import { create } from "apisauce"; | ||
|
||
const apiClient = create({ | ||
baseURL: "https://ill-gold-parrot-sock.cyclic.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.
I would prefer to place endpoints and other details in environment (.env) file or Config.
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.
I was thinking of refactoring this later, but why not now!?!.. updated :)
app/api/hooks/useApi.js
Outdated
const [data, setData] = useState([]); | ||
const [error, setError] = useState(false); | ||
const [loading, setLoading] = useState(false); |
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.
These 3 state can be combined to one say status
and we can reuse the AsyncStatus logic here. which I described above.
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.
Updated
const login = (username, password) => | ||
client.post("/login", { username, password }); | ||
|
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.
Non-blocking comment
It is okay for now but we should also add validation logic before service call.
For eg: validate username and password are not null, regex (if any) etc...
-> This will help to prevent service call for which backend will throw error.
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.
Yes, will create a separate dedicated PR for validation.
|
||
import AsyncStorage from "@react-native-async-storage/async-storage"; | ||
|
||
const storeData = async (key, value) => { |
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.
This file is interesting and liked it, would like to understand the running scenarios.
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.
Yeah, lets discuss scenarios during our next sync-up
@@ -11,7 +18,10 @@ import { SafeAreaView, StyleSheet, View } from "react-native"; | |||
function Screen({ children, style }) { | |||
return ( | |||
<SafeAreaView style={[styles.screen, style]}> | |||
<View style={style}>{children}</View> | |||
<StatusBar barStyle={"dark-content"} /> | |||
<TouchableWithoutFeedback onPress={() => Keyboard.dismiss()}> |
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.
+1
function authenticate() { | ||
authContext.setUser({}); | ||
} | ||
const keyboardVerticalOffset = Platform.OS === "ios" ? 20 : 0; |
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.
+1 for thinking about this behavior.
b56f381
to
28cc16d
Compare
App.js
Outdated
function renderSwitch() { | ||
switch (appStatus) { | ||
case AsyncStatus.Succeeded: | ||
return <>{user ? <AppNavigator /> : <AuthNavigator />}</>; |
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.
<></> i not required can be skipped.
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.
Updated
|
||
const request = async (...args) => { | ||
setApiStatus(AsyncStatus.Loading); | ||
const response = await apiFunc(...args); |
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.
Let’s use promises for this.
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.
It is okay to address in next pr
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.
Ok, will update that in next PR
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.
I tried implementing the promise here just now but was facing some errors and the code started looking a little ugly. I would suggest we use async-await along with try-catch to handle all scenarios. With async-await, the code looks more maintainable and organized as compared to promise. I would like to hear your concerns about async-await. Let's discuss this during our sync up.
}; | ||
|
||
const getCurrentSettings = () => { | ||
if (__DEV__) return settings.dev; |
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.
Where will we store this DEV does it mean from environment variable? Or from webpack?
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.
DEV is a global variable from react native packager. It is set during runtime by react native and is available to use anywhere in the app for encapsulating environment specific code blocks.
28cc16d
to
b21d07b
Compare
This PR -
Simulator.Screen.Recording.-.iPhone.12.Pro.Max.-.2023-05-18.at.22.02.01.mp4
Coming up in next PR -