diff --git a/src/App.jsx b/src/App.jsx index 96d5a29..4124286 100644 --- a/src/App.jsx +++ b/src/App.jsx @@ -17,6 +17,11 @@ export default function App() { // ================================ // States to store data + // While user is a state you might need all across the app and it is fine to store in the most top-level component without Context or Redux, + // I think that state like balance, which is more of a local state to the Account component, it should be then kept there as well. Or rather kept with the form. It is always a bit tricky however with these vanilla React projects and gets easier later on with proper tools. Form libraries, state management libraries etc. + // Ultimately you should decide where to store state depending on its role in regards to the whole app. + // Do you need the state across multiple sibling components? Storing it in the parent is good. + // Is the state only relevant for a single component and possible its children? Then not necessary to store the state in its parent component. const [user, setUser] = useState({}); const [balance, setBalance] = useState({}); const [requests, setRequests] = useState([]); @@ -75,8 +80,29 @@ export default function App() { setRequests([...existingRequests]); }; + // This is not a good pattern. This is a component and should therefore be defined outside of the App component. It could use the user and balance as props if you had to use it. // to determing which account dashboard component to render (Leader or Member) const renderAccount = () => { + /* + I think this would be a bit nicer to read: + + if (![1,2].includes) return
Error
; + if (user.role === 2) return ; + return (
...
) + + If you could rethink your database types for the role column, I think it would be even better. I would rather make it ENUM strings, e.g. "USER" and "LEADER" or whatever. + + Then you could compare with an enum object: + const UserRole = { + USER: "USER", + LEADER: "LEADER" + }; + + user.role === UserRole.LEADER + + That way it is easier to read and people who don't know your code have an easier time understanding as well. + + */ if (user.role === 1) { return (
@@ -120,6 +146,7 @@ export default function App() {
{renderCreateButton()} + {/* {showCreate &&
} */} {showCreate ? : <>}
diff --git a/src/components/Account.jsx b/src/components/Account.jsx index 8bfcc13..eaccc5a 100644 --- a/src/components/Account.jsx +++ b/src/components/Account.jsx @@ -8,6 +8,9 @@ export default function Account({ user, balance }) { // ================================ // RENDERING OF COMPONENT // ================================ + + // are all user properties always filled out by the user? Is everything required in the db and on the frontend? + // If no, then you could potentially render more than necessary here. return (
diff --git a/src/components/Form.jsx b/src/components/Form.jsx index 077f8b2..dfde568 100644 --- a/src/components/Form.jsx +++ b/src/components/Form.jsx @@ -18,6 +18,11 @@ export default function Form({ // HELPER FUNCTIONS // ================================ + + // all these handlers can also be omitted by using the following pattern: + // setDate(e.target.value)} + // Instead of using a named handler function, we can use an anonymous function within the onClick and define within the block what it will do. + // ^this works like vanilla js click handlers with anonymous functions. element.addEventListener("click", (e) => { ... }) const handleChangeDate = (event) => { const newDate = event.target.value; setDate(newDate); @@ -41,20 +46,24 @@ export default function Form({ user, date, leaveType, comment, }) .then((response) => { + // console logs should be removed for production console.log('request created in DB'); toggleForm(); // get updated list of requests axios .post('/getRequests', user) .then((response2) => { + // what is data is undefined? what if [0] index is undefined? updateRequestList(response2.data.allExistingRequests[0]); // get updated leave balance axios .post('/getLeavebalance', user) .then((response3) => { + // what if data or leaveBalance is undefined? updateBalance({ ...response3.data.leaveBalance.balance }); }); }); }) + // if all we do is log the error, the client will not get any feedback. Could get a bit creative here with error messages being displayed to the user. .catch((error) => { console.log(error); }); }; @@ -69,6 +78,7 @@ export default function Form({