Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 27 additions & 0 deletions src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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([]);
Expand Down Expand Up @@ -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 <div>Error</div>;
if (user.role === 2) return <LeaderAccount ... />;
return (<div>...<div>)

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 (
<div>
Expand Down Expand Up @@ -120,6 +146,7 @@ export default function App() {
</div>
<div id="form-container" className="container-sm">
{renderCreateButton()}
{/* {showCreate && <Form ... />} */}
{showCreate ? <Form user={user} toggleForm={toggleForm} updateRequestList={updateRequestList} updateBalance={updateBalance} /> : <></>}
</div>
<div id="request-container" className="container-sm">
Expand Down
3 changes: 3 additions & 0 deletions src/components/Account.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<div id="leave-container" className="container-sm">
<div className="row">
Expand Down
10 changes: 10 additions & 0 deletions src/components/Form.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ export default function Form({
// HELPER FUNCTIONS
// ================================


// all these handlers can also be omitted by using the following pattern:
// <Component onClick={(e) => 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);
Expand All @@ -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); });
};

Expand All @@ -69,6 +78,7 @@ export default function Form({
</div>
<div className="input-group">
<select id="leave-input" type="text" placeholder="Type of Leave" className="form-select" value={leaveType} onChange={handleChangeLeaveType}>
{/* Could refactor the options also into a function if you wanted to. Make a list of the values and strings, then iterate over it and return JSX for each. */}
<option>Please Choose Type of Leave</option>
<option value="AL">Annual Leave</option>
<option value="SL">Sick Leave</option>
Expand Down
7 changes: 7 additions & 0 deletions src/components/LoginForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ export default function LoginForm({
if (response.data.status && response.data.user.role === 1) {
updateUser(response.data.user);
// get leave balance of member
// instead of these nested API calls, I think we could rather attach includes to our request or since we know on login we will want to attach the leave balance and requests, might as well just attach it.
// Response:
// { User, LeaveBalance, Requests }
// If you wanted to dynamically add these includes, maybe because multiple components query the same route, you could include it in your request:
// POST /attemptLogin?include=[LeaveBalance]
// then in our API you pick that up
// Sequelize.User.find({ include: [...request.params.include]}) or something along these lines.
axios
.post('/getLeavebalance', response.data.user)
.then((response2) => {
Expand Down
18 changes: 18 additions & 0 deletions src/components/Table.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ export default function Table({
}) {
// helper functions
const nameOfLeave = (type) => {
// I think a switch statement would work a bit nicer or even better an object.
/*
const leaveTypes = {
AL: 'Annual Leave',
CL: 'Childcare Leave',
}

leaveTypes[type] // if type is AL should be 'Annual Leave', if no match then undefined, which you could make an Error then.

*/
if (type === 'AL') {
return 'Annual Leave';
}
Expand Down Expand Up @@ -61,6 +71,13 @@ export default function Table({
};

const updateRequest = (leaveRequest, newStatus) => {
// instead of always importing axios everywhere, you could define a POST, GET, PUT etc. function
/* const postRequest = async (url, payload) => {
const response = await axios.post(url, payload);
return response;
}
That way you avoid repeating yourself over and over.
*/
axios
// posting to Bill DB
.post('/updateRequest', { leaveRequest, newStatus })
Expand Down Expand Up @@ -140,6 +157,7 @@ export default function Table({
<h2>All Requests</h2>
</div>
<div className="col-12" id="newtable">
{/* Using tables is quite oldschool. Nowadays flex and/or grid is advised */}
<table className="table table-striped table-bordered table-hover table-sm align-middle table-responsive">
<thead>
<tr className="table-dark">
Expand Down