Skip to content

Comments

PTBC9 Project 1 - Qianling#40

Open
Qiannnly wants to merge 92 commits intorocketacademy:mainfrom
Qiannnly:main
Open

PTBC9 Project 1 - Qianling#40
Qiannnly wants to merge 92 commits intorocketacademy:mainfrom
Qiannnly:main

Conversation

@Qiannnly
Copy link

No description provided.

@@ -13,3 +13,23 @@ Open [http://localhost:3000](http://localhost:3000) to view it in your browser.

Copy link

Choose a reason for hiding this comment

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

Would be great to also include install instructions!

"@emotion/styled": "^11.11.0",
"@mui/icons-material": "^5.14.16",
"@mui/material": "^5.14.17",
"gh-pages": "^6.1.0",
Copy link

Choose a reason for hiding this comment

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

I think this is probably a dev dependency. Can try using npm install -D gh-pages to install as dev dependency

justifyContent: "center",
alignItems: "center",
padding: "5px 20px",
// bgcolor: "#daa06d",
Copy link

Choose a reason for hiding this comment

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

let's remove comments if not needed

import HomeIcon from "@mui/icons-material/Home";
import { useNavigate } from "react-router-dom";

const modalStyles = {
Copy link

Choose a reason for hiding this comment

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

nice job defining this outside of the component

setIsTransactionFormShown,
}) => {
const [showMessage, setShowMessage] = useState([]);
const [open, setOpen] = useState(false);
Copy link

Choose a reason for hiding this comment

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

I would rather name this isOpen like a boolean

};

const handleSubmit = () => {
if (!activity || !items) {
Copy link

Choose a reason for hiding this comment

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

I like your early returns, very good practice!

Comment on lines +7 to +9
if (getActivityData) {
return true;
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (getActivityData) {
return true;
}
if (getActivityData) return true;

curly braces are not necessary for one liners, feel free to use it if you like it

Comment on lines +14 to +18
const getActivityData = JSON.parse(localStorage.getItem("activityDetails"));

const [activityDetails, setActivityDetails] = useState(
getActivityData ? getActivityData : { activity: "", items: "" }
);
Copy link

Choose a reason for hiding this comment

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

Suggested change
const getActivityData = JSON.parse(localStorage.getItem("activityDetails"));
const [activityDetails, setActivityDetails] = useState(
getActivityData ? getActivityData : { activity: "", items: "" }
);
const [activityDetails, setActivityDetails] = useState(
JSON.parse(localStorage.getItem("activityDetails")) || { activity: "", items: "" }
);

Comment on lines +23 to +43
return (
<>
{!isTransactionFormShown ? (
<>
<CreateForm
setActivityDetails={setActivityDetails}
setIsTransactionFormShown={setIsTransactionFormShown}
/>
</>
) : (
<>
<div style={{ overflow: "auto", height: "50vh" }}>
<AddTransactionsForm
activityDetails={activityDetails}
setIsTransactionFormShown={setIsTransactionFormShown}
/>
</div>
</>
)}
</>
);
Copy link

Choose a reason for hiding this comment

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

Suggested change
return (
<>
{!isTransactionFormShown ? (
<>
<CreateForm
setActivityDetails={setActivityDetails}
setIsTransactionFormShown={setIsTransactionFormShown}
/>
</>
) : (
<>
<div style={{ overflow: "auto", height: "50vh" }}>
<AddTransactionsForm
activityDetails={activityDetails}
setIsTransactionFormShown={setIsTransactionFormShown}
/>
</div>
</>
)}
</>
);
if (!isTransactionFormShown) return <CreateForm ... />
return (
<div style={{ overflow: "auto", height: "50vh" }}>
<AddTransactionsForm
activityDetails={activityDetails}
setIsTransactionFormShown={setIsTransactionFormShown}
/>
</div>
);

Copy link

Choose a reason for hiding this comment

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

We can also return early for components to improve readability

return greetings;
};

let greetMessage = `${greetUser()} ${username},`;
Copy link

Choose a reason for hiding this comment

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

Suggested change
let greetMessage = `${greetUser()} ${username},`;
const greetMessage = `${greetUser()} ${username},`;

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