Skip to content
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

Feature/authentication #3

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Feature/authentication #3

wants to merge 21 commits into from

Conversation

Waysker
Copy link
Collaborator

@Waysker Waysker commented Aug 17, 2020

Basic working authentication, to be reviewed and merged. Works on a demo API which provides user registration, authorization and email recovery.

@Waysker Waysker requested review from pectom and mon8-lig August 17, 2020 13:43
src/modules/navigation/MainNavigator.js Show resolved Hide resolved
src/modules/services/Auth.js Outdated Show resolved Hide resolved
src/modules/services/Auth.js Outdated Show resolved Hide resolved
Comment on lines 51 to 56
<Form
title={formProps.title}
fields={formProps.fields}
onSubmit={formProps.onSubmit}
loading={formProps.loading}
>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Przy wpisywaniu loginu znika pole z hasłem i dość dziwnie to wygląda (analogiczna sytuacja z wpisywaniem hasła, znika button "Login")
Screen z symulatora:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

U mnie się tak nie dzieje ale mam pewne podejrzenie. Jesteś w stanie sprawdzić czy wysuwająca się klawiatura nie ma nad sobą pewnego białego obszaru ? Jeśli tak to pewnie on zasłania to co jest pod

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sprawdziłam, rzeczywiście jest to biały obszar nad wysuwającą się klawiaturą. Gorzej że na Androidzie na moim telefonie występuje ten sam problem

src/views/authentication/Login.js Outdated Show resolved Hide resolved
src/views/authentication/ForgotPassword.js Outdated Show resolved Hide resolved
src/constants/endpoints/auth.js Outdated Show resolved Hide resolved
src/views/authentication/AuthLoading.js Outdated Show resolved Hide resolved
const [error, setError] = useState(null);
const [loading, setLoading] = useState(false);

const fields = [{ name: "email", label: "Email Address", required: true }];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

przydałaby się walidacja wpisanej wartości do pola email. Formik ma łatwą obsługę walidacji jbc

Copy link
Contributor

@pectom pectom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bardzo dużo zmian i super robota! Nie powiedziałbym, że to jest pierwszym kod w RN. Trochę jest do zmiany, żeby to domergować. Najważniejsze, żeby to flow w pełni działalo i nie było żadnych jaj z nawigacją.

src/modules/navigation/MainNavigator.js Show resolved Hide resolved
src/modules/context/Auth.js Outdated Show resolved Hide resolved
src/components/authentication/CTA.js Outdated Show resolved Hide resolved
src/components/authentication/Shared.js Outdated Show resolved Hide resolved
src/constants/endpoints/auth.js Outdated Show resolved Hide resolved
src/constants/styles/typography.js Outdated Show resolved Hide resolved
src/modules/context/Auth.js Outdated Show resolved Hide resolved
const AuthStack = createStackNavigator();

const AuthStackScreen = () => (
<AuthStack.Navigator>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tutaj potrzeba zmiany flow, bo nie może być tak, że cofamy się do widoku AuthLoading z jakiegoś innego ekranu. Nie wiem jak to zrobić teraz, bo api się react navigation bardzo się zmieniło. Jeśli nie widziałeś jeszcze to może przejrzyj dokumentację od nich link.

Coś co mi przyszło do głowy to schowanie headera, ale i tak nie pomaga, bo dalej możemy cofać przyciskiem/gestami. Zamieszczam jako ciekawostkę.

    <AppStack.Navigator
      screenOptions={{
        headerShown: false,
      }}
    >
      <AppStack.Screen name="Loading" component={AuthLoading} />
      <AppStack.Screen name="Home" component={HomeScreen} />
      <AppStack.Screen name="Auth" component={AuthStack} />
    </AppStack.Navigator>
    <AuthStack.Navigator>
    <AuthStack.Screen
      options={{ headerShown: false }}
      name="Login"
      component={LoginScreen}
    />
    <AuthStack.Screen name="Register" component={RegisterScreen} />
    <AuthStack.Screen name="ForgotPassword" component={ForgotPasswordScreen} />
  </AuthStack.Navigator>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tego byłem świadom właśnie, próbowałem coś dziobać zgodnie z tym linkiem wcześniej ale nie poszło. Jeszcze do tego wrócę

src/views/authentication/Login.js Outdated Show resolved Hide resolved
src/modules/services/Auth.js Outdated Show resolved Hide resolved
@Waysker
Copy link
Collaborator Author

Waysker commented Aug 21, 2020

Nie zmieniłem jeszcze flow oraz nie ogarnąłem tej walidacji emailem. Prawdopodobnie zrobię to dopiero po powrocie :)

…th styles. TODO same for Login.js and ForgotPassword.js
@Waysker Waysker requested review from pectom and mon8-lig October 18, 2020 11:55
@pectom pectom force-pushed the feature/authentication branch from 04d223e to 8fa265c Compare October 19, 2020 22:15
Copy link
Contributor

@pectom pectom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super, że to flow już działa, ale pasuje zrobić kilka poprawek przed domergowaniem do mastera. Zerknij na te komentarze i daj znać na kiedy byś mógł to zrobić. Szczególnie zwróć uwagę na te nieużywane rzeczy, bo pewnie nie wychwyciłem wszystkiego. Raczej są to małe poprawki. Z większych rzeczy, których nie ująłem zrobiłem taski na trello i to już wrzucimy w kolejnym PR'ach.

  • podpięcie bazy danych do procesu uwierzytelniania
  • poprawa stylu ekranów uwierzytelniania (falka na dole)

src/components/common/AppButton.js Outdated Show resolved Hide resolved
src/api/Auth.js Show resolved Hide resolved
src/constants/styles/colors.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/components/authentication/Shared.js Outdated Show resolved Hide resolved

// check if username is null
const username = response.user.username !== null;
if (username);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brakuje jakiejś akcji jeśli warunek jest prawdziwy

if (username);
else navigation.replace("Username");
} catch (err) {
Alert.alert("Login Unsuccessful", err.message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J.polski

onPress: PropTypes.func,
style: ViewPropTypes.style,
titleStyle: ViewPropTypes.style,
ctaStyle: ViewPropTypes.style.Text,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coś jest nie tak z tym typem. Dostaję tutaj warning

const { navigation } = props;

// 1 - DECLARE VARIABLES
const [setError] = useState(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wywala błąd setError is not a function. W tablicy musi być podana wartość i funkcja, która ma ją zmieniać.

const { navigation } = props;

// 1 - DECLARE VARIABLES
const [setError] = useState(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Błędne użycie useState, podobnie jak w rejestracji.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants