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

Calendar functionality #18

Closed
wants to merge 31 commits into from
Closed

Conversation

VJagiasi
Copy link
Collaborator

@VJagiasi VJagiasi commented Jul 5, 2024

Notion ticket link

Calendar Absence Creation and Deletion

Implementation description

  • Implemented a fully functional calendar with the ability to add and delete absences which gets stored in the database and is in sync

Steps to test

run this in light mode as it's not styled yet completely for dark mode

  1. Ensure the database is running
  2. npm run dev and go to localhost:3000/calendar
  3. Try adding and deleting multiple absences and check if they are getting updated in the database

What should reviewers focus on?

  • Absences getting synced with the database and the calendar on the frontend

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

prisma/schema.prisma Outdated Show resolved Hide resolved
src/pages/calendar.tsx Outdated Show resolved Hide resolved
placeholder="Enter substitute teacher ID"
value={substituteTeacherId}
onChange={(e) => setSubstituteTeacherId(e.target.value)}
color="black"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this might not matter because this ticket is more about frontend than styling, but on these number inputs if you scroll on your trackpad the number changes which is annoying

Copy link
Collaborator Author

@VJagiasi VJagiasi Jul 20, 2024

Choose a reason for hiding this comment

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

doesn't seem to happen when I try to fill out the input form, do you mean it just starts changing as soon as you scroll?

FetchEvents();
}, []);

const FetchEvents = async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this should fetch from the db based on if its in day, week, or month mode. I feel like this would be better than grabbing all absences in the db, especially if there are be lot of absences. The useEffect can run this function every time the view is changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but we anyways have to fetch them initially for month view in the beginning so won't it be unnecessary to fetch multiple times for different views

@phamkelly17
Copy link
Collaborator

phamkelly17 commented Jul 10, 2024

Couldn't actually test functionality because I'm getting this error. So I'm wondering if you've gotten this same error.
Screenshot 2024-07-09 at 9 31 47 PM

My main suggestion is how all the Absences are called Events. Imo event is a very general term and they're all absences so I think it could be clearer if all those variables were called Absences

Copy link
Member

@ChinemeremChigbo ChinemeremChigbo left a comment

Choose a reason for hiding this comment

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

I think a lot of unnecessary files where changed during your rebasing / merging with main. I would take another look to make sure only the changes you intend to make are being made.

.env.sample Outdated Show resolved Hide resolved
.prettierignore 2 Outdated Show resolved Hide resolved
.husky/pre-commit Outdated Show resolved Hide resolved
.prettierrc 2 Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
pnpm-lock.yaml Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
src/pages/index.tsx Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
Copy link
Member

@ChinemeremChigbo ChinemeremChigbo left a comment

Choose a reason for hiding this comment

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

Add steps for seeding to Steps to test

@@ -17,8 +17,7 @@ RUN npm install
EXPOSE 3000

# Run the application
CMD echo "Waiting for database to be ready..." && \
echo "Running Prisma commands..." && \
CMD echo "Running Prisma commands..." && \
Copy link
Member

Choose a reason for hiding this comment

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

Revert changes here.

Copy link
Member

Choose a reason for hiding this comment

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

Revert changes here

@@ -44,7 +44,7 @@
"@types/react-dom": "^18.0.0",
"@typescript-eslint/eslint-plugin": "^8.8.0",
"@typescript-eslint/parser": "^8.8.0",
"eslint": "^8.57.1",
"eslint": "^8.57.0",
Copy link
Member

Choose a reason for hiding this comment

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

Revert

"prisma": "^5.16.0",
"postgres": "^3.4.4",
Copy link
Member

Choose a reason for hiding this comment

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

Revert

"@mui/material": "^5.15.20",
"@mui/system": "^5.15.20",
"@faker-js/faker": "^8.4.1",
Copy link
Member

Choose a reason for hiding this comment

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

Revert

@@ -2,7 +2,7 @@
"private": true,
"scripts": {
"dev": "next dev",
"build": "next build",
"build": "prisma generate && prisma migrate && next build",
Copy link
Member

Choose a reason for hiding this comment

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

Revert

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.

5 participants