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

Yuji Project 1 PTBC5 #11

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Yuji Project 1 PTBC5 #11

wants to merge 25 commits into from

Conversation

Flashrob
Copy link

No description provided.

Copy link
Author

@Flashrob Flashrob left a comment

Choose a reason for hiding this comment

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

@yuj8fuj6 here your code review

@@ -1,4 +1,4 @@
# Rocket Academy Coding Bootcamp: Project 1: Frontend App
# Project 1: Frontend App - Aime (WIP)

Copy link
Author

Choose a reason for hiding this comment

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

It would be good to include instructions on how to install and run the app. Also a description of your project, as detailed as you like - but at least some kind of overview.

Comment on lines +21 to +23
// const [users, setUsers] = useLocalStorage("users", []);

// setUsers([...users, "Yuji"])
Copy link
Author

Choose a reason for hiding this comment

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

Let's remove unused code for our "production" code

import { BsExclamationDiamond } from "react-icons/bs";

const AlertButton = (props) => {
const phoneNumber = [];
Copy link
Author

Choose a reason for hiding this comment

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

This seems non-functional to me. I think it would be good to make it work with the user's phone number, even if you only have 1 user as of now.

import { BsArrowLeftCircle } from "react-icons/bs";

const BackButton = () => {
const [isClicked, setIsClicked] = useState(false);
Copy link
Author

Choose a reason for hiding this comment

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

What is this used for? Seems redundant to me

@@ -0,0 +1,21 @@
import React, { useState } from "react";
import { Link, NavLink } from "react-router-dom";
Copy link
Author

Choose a reason for hiding this comment

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

Is NavLink used?

Comment on lines +25 to +40
const [name, setName] = useState("");
const [age, setAge] = useState("");
const [weight, setWeight] = useState("");
const [bloodType, setBloodType] = useState("");
const [medicalConditions, setMedicalConditions] = useState("");
const [allergies, setAllergies] = useState("");
const [phone, setPhone] = useState("");
const [email, setEmail] = useState("");
const [emer1Name, setEmer1Name] = useState("");
const [emer1Phone, setEmer1Phone] = useState("");
const [emer1Email, setEmer1Email] = useState("");
const [emer2Name, setEmer2Name] = useState("");
const [emer2Phone, setEmer2Phone] = useState("");
const [emer2Email, setEmer2Email] = useState("");
const [userName, setUserName] = useState("");
const [password, setPassword] = useState("");
Copy link
Author

Choose a reason for hiding this comment

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

One user object should suffice.

Comment on lines +46 to +61
setName(profileData[index].name);
setAge(profileData[index].age);
setWeight(profileData[index].weight);
setBloodType(profileData[index].bloodType);
setMedicalConditions(profileData[index].medicalConditions);
setAllergies(profileData[index].allergies);
setPhone(profileData[index].phone);
setEmail(profileData[index].email);
setEmer1Name(profileData[index].emer1Name);
setEmer1Phone(profileData[index].emer1Phone);
setEmer1Email(profileData[index].emer1Email);
setEmer2Name(profileData[index].emer2Name);
setEmer2Phone(profileData[index].emer2Phone);
setEmer2Email(profileData[index].emer2Email);
setUserName(profileData[index].userName);
setPassword(profileData[index].password);
Copy link
Author

Choose a reason for hiding this comment

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

That is a whole lot of state updates. Let's reduce it to 1 by using an object.

Comment on lines +166 to +181
name={name}
age={age}
weight={weight}
bloodType={bloodType}
medicalConditions={medicalConditions}
allergies={allergies}
phone={phone}
email={email}
emer1Name={emer1Name}
emer1Phone={emer1Phone}
emer1Email={emer1Email}
emer2Name={emer2Name}
emer2Phone={emer2Phone}
emer2Email={emer2Email}
userName={userName}
password={password}
Copy link
Author

Choose a reason for hiding this comment

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

Reiterating on my previous point of using an object. When you look at it, and it seems overwhelming, and while writing it, it feels like a big chore - then let's refactor it into a better data structure :)! Engineers should strive to be lazy and do as little manual work as possible haha

import { Navigate } from "react-router-dom";
import appicon from "../data/appicon.png";

const Start = () => {
Copy link
Author

Choose a reason for hiding this comment

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

Not a code, but product comment. I thought this start screen was a bit redundant :D!

<Link to="/home">Record</Link>
</button>
<Link to="/home">
<Button value="To Main" />
Copy link
Author

Choose a reason for hiding this comment

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

I found this button really odd tbh. You can register, or just bypass registration. I think it should either be no login required, or login enforced. This here is an in-between. If this is for development purposes, let's either render it only in development environment, or remove it before we push to production app.

"chart.js": "^3.9.1",
"chartjs-adapter-moment": "^1.0.0",
"date-fns": "^2.29.3",
"lowdb": "^3.0.0",
Copy link
Author

Choose a reason for hiding this comment

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

let's remove libraries, if we don't use them!

BsEmojiFrown,
} from "react-icons/bs";

const FormMood = (props) => {
Copy link
Author

Choose a reason for hiding this comment

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

example of using ...props to get all remaining props

Suggested change
const FormMood = (props) => {
const FormMood = ({ children, value, text, ...props }) => {

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.

2 participants