Skip to content

Ian's Code Review #46

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.

Sign up for GitHub

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

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

Ian's Code Review #46

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
637b8c0
fixed typo
taliabahar Mar 23, 2021
9039657
Code Review - Added comments for review
ianlizzo Mar 24, 2021
04cbf47
Code Review - Added comments
ianlizzo Mar 24, 2021
52240c9
Updated review
ianlizzo Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions src/screens/EmergencyContacts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,34 @@
import React, { useState } from "react";
import { StyleSheet, Text, View, TouchableOpacity } from "react-native";
import { TextInput } from "react-native-gesture-handler";
import { registerNewUser } from "../../firebase/firebase.util";
import { colors } from "../styles/colors.js";
import { AppLoading } from "expo";
import {
useFonts,
Nunito_600SemiBold,
Nunito_400Regular,
Nunito_600SemiBold,
Nunito_800ExtraBold,
useFonts,
} from "@expo-google-fonts/nunito";
import { CoveredByYourGrace_400Regular } from "@expo-google-fonts/covered-by-your-grace";
import {
// Quicksand_300Light,
Quicksand_400Regular,
Quicksand_500Medium,
Quicksand_600SemiBold,
Quicksand_700Bold,
} from "@expo-google-fonts/quicksand";
import React, { useState } from "react";
import { StyleSheet, Text, TouchableOpacity, View } from "react-native";

import { AppLoading } from "expo";
import { CoveredByYourGrace_400Regular } from "@expo-google-fonts/covered-by-your-grace";
import { TextInput } from "react-native-gesture-handler";
Copy link
Collaborator

@ianlizzo ianlizzo Mar 24, 2021

Choose a reason for hiding this comment

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

Make sure this is the TextInput you want, usually the base one is imported from "react-native"

import { colors } from "../styles/colors.js";
import { registerNewUser } from "../../firebase/firebase.util";

// TODO: remove extraneous comments
const EmergencyContacts = ({ navigation }) => {
// [Ian]: I would recommend updating to React Navigation 5.x.x so you can use Hooks and
Copy link
Collaborator

@ianlizzo ianlizzo Mar 24, 2021

Choose a reason for hiding this comment

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

To add to this, I recommend Hooks so that you don't have to pass the prop around constantly. For example, instead of passing in the navigation prop, you would use

Suggested change
// [Ian]: I would recommend updating to React Navigation 5.x.x so you can use Hooks and
const navigation = useNavigation();
...
navigation.navigate(someRoute)

and then for your getParam you could use the route hook:

Suggested change
// [Ian]: I would recommend updating to React Navigation 5.x.x so you can use Hooks and
const route = useRoute();
const { object, otherObject } = route.params;

// reduce the ability to run into any deprecating errors.
let object = navigation.getParam("object", "missing");
const [name, setName] = useState("");
const [contactPhone, setContactPhone] = useState("");
// [Ian]: I'm not well versed in this package, but if you find yourself setting fonts
// and waiting for loading in your component, I would recommend maybe rendering
// it at the top level with your App.js (if possible).
let [fontsLoaded] = useFonts({
Nunito_400Regular,
Nunito_600SemiBold,
Expand All @@ -39,11 +44,13 @@ const EmergencyContacts = ({ navigation }) => {
return (
<View style={styles.container}>
<Text style={styles.header}>
Who you would like to call during an Emergency?
Who would you like to call during an Emergency?
{/* TODO: change wording */}
</Text>
<Text style={styles.taskText}>Name</Text>
<TextInput
// [Ian]: I would try and keep your styling consistent, and add it to
// your Stylesheet.
style={{
height: 40,
width: "75%",
Expand Down Expand Up @@ -99,8 +106,11 @@ const EmergencyContacts = ({ navigation }) => {
}
};

// [Ian]: This function should be moved above the `return` to keep with React convention.
// [Ian]: I would also change the name of the function as it is a bit confusing.
function objectifyAndNav(navigation, object, name, contactPhone) {
// add new items to our object
// [Ian]: I would also try to use something more verbose and explicit instead of `object`.
object.set("eName", name);
object.set("eNumber", contactPhone);

Expand Down