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

Patrick-HongYun-Project3-Frontend #47

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

Conversation

patrickkok
Copy link

No description provided.

magiicloud and others added 30 commits March 20, 2024 17:03
…ils. also able to select item name and pull out serial num
Manage item form to update cycle count values
No conflicts, remember to remove unused console.log statements in the future
patrickkok and others added 28 commits April 10, 2024 12:39
added sheets to rooms add items
…or the current user, add select menu for buildings, Add qty on itms in rooms
Only show items that is in the user's building
Update readme with project planning information
amended login layout and added protected route to login
…rror: 'originalKeywordKind' has been deprecated since v5.0.0 and can no longer be used
Comment on lines +1 to +31
// import React from "react";
// import { BrowserRouter, Routes, Route, Navigate } from "react-router-dom";
// import { Login } from "./pages/Login";
// import { MenuFrame } from "./pages/MenuFrame";
// import { Buildings } from "./pages/Buildings";
// import { Room } from "./pages/Room";
// import { ManageItems } from "./pages/ManageItems";
// import { AddNewItem } from "./pages/AddNewItem";
// import { AllItems } from "./pages/allitems/page";

// const App = () => {
// return (
// <BrowserRouter>
// <Routes>
// <Route path="/" element={<Login />} />
// <Route path="/landing" element={<MenuFrame />}>
// {/* Nested routes will render within <MenuFrame /> */}
// <Route path="buildings" element={<Buildings />} />
// <Route path="room" element={<Room />} />
// <Route path="allitems" element={<AllItems />} />
// <Route path="manageitems" element={<ManageItems />} />
// <Route path="addnewitem" element={<AddNewItem />} />
// </Route>
// {/* Redirect to "/landing/allitems" or a default nested route if "/landing" is accessed directly */}
// <Route
// path="/landing"
// element={<Navigate replace to="/landing/allitems" />}
// />
// </Routes>
// </BrowserRouter>
// );

Choose a reason for hiding this comment

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

U know what I want to comment here, right? :D

<Route path="/" element={<Login />} />
{/* Apply the ProtectedRoute component here to protect all nested routes */}
<Route
path="/landing"

Choose a reason for hiding this comment

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

While it is nice to protect all the components in this manner, I find the routing of prefixing every page with /landing questionable!


import { cn } from "../../lib/utils";

const Avatar = React.forwardRef<

Choose a reason for hiding this comment

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

Why do u need a ref on an image? I am a bit confused

@@ -0,0 +1,48 @@
import * as React from "react";

Choose a reason for hiding this comment

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

All files in your ui folder are components. So those should be in capital letter

@@ -0,0 +1,92 @@
Geist Sans and Geist Mono Font

Choose a reason for hiding this comment

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

Do you really need ALL these fonts?

<TableBody>
{allItems.map((item) => (
<TableRow key={item.id}>
<TableCell>{item.serial_num}</TableCell>

Choose a reason for hiding this comment

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

Having a TableItem component would be nice here for abstraction

Comment on lines +101 to +104
form.setValue("itemName", response.data.serial_num);
form.setValue("quantity", response.data.roomItems[0].quantity);
form.setValue(
"expiryDate",

Choose a reason for hiding this comment

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

Is there a way to set multiple form values at once?

Comment on lines +55 to +56
const [expCount, setExpCount] = useState(0);
const [expItems, setExpItems] = useState<ExpItem[]>([]);

Choose a reason for hiding this comment

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

A single object would work here, which contains the count and the items

Choose a reason for hiding this comment

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

Also, naming this exp can mean many different things. I would try to be more clear here

const [parCount, setParCount] = useState(0);
const [parItems, setParItems] = useState<ParItem[]>([]);
const [buildings, setBuildings] = useState<buildingList>([]);
const [currentBuilding, setCurrentBuilding] = useState<building | undefined>(

Choose a reason for hiding this comment

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

are we using undefined here only for the default value? If so, why not use an empty building object here instead?

const onSubmit = async (formData: z.infer<typeof formSchema>) => {
if (form.getValues("type") === "deleteroomitem") {
try {
const response = await sendRequest(`/deleteroomitem/`, {

Choose a reason for hiding this comment

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

that route seems not in line with REST patterns. Maybe look that up once more and find a more appropriate route

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