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

Create admin schedule review table #206

Merged
merged 8 commits into from
Apr 1, 2022
Merged

Conversation

7tint
Copy link
Member

@7tint 7tint commented Mar 13, 2022

Ticket link

Closes #156
Closes #158

Implementation description

  • Implemented the schedule review table and its table rows, which display all volunteer shifts of a certain week.
  • Note that this table implementation is only for displaying the shifts in one week (passed in via props), and does not include the date navigation header.

Steps to test

  1. Add the following code to Default.tsx, importing components/variables when necessary:
<Container maxW="container.xl">
  <AdminScheduleTable schedule={TableTestData} />
</Container>
  1. Check http://localhost:3000/ to see that TableTestData is rendered correctly. The test data includes three types of table rows, as specified in Schedule review table row #156.

What should reviewers focus on?

  • Clean React code, correct separation of concern for the props & states.
  • Stay hydrated 💧

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

@github-actions
Copy link

github-actions bot commented Mar 13, 2022

Visit the preview URL for this PR (updated for commit 47eafa0):

https://sistering-dev--pr206-lambert-schedule-rev-gzcm6kx1.web.app

(expires Thu, 07 Apr 2022 23:00:34 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@7tint 7tint changed the title Lambert/schedule review table Implement admin schedule review table Mar 13, 2022
@7tint 7tint changed the title Implement admin schedule review table Create admin schedule review table Mar 13, 2022
Copy link
Member

@LenaNguyen LenaNguyen left a comment

Choose a reason for hiding this comment

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

padding for the rows is a little off
image

I'm pretty sure the horizontal padding should be equal so let's use 47px.
Since the Td elements have their own padding and it would be annoying to remove their padding each time, just add some extra padding to tr. i.e. px="20px", py="2.5px"

Copy link
Member

@LenaNguyen LenaNguyen left a comment

Choose a reason for hiding this comment

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

Also for UI changes can you pls add a demo on the default page or something next time 🙏 my docker takes a million years to start so sometimes it's much faster to use the firebase preview. The downside is you'd have to remove it after get another approval tho.

@LenaNguyen
Copy link
Member

padding for the rows is a little off image

I'm pretty sure the horizontal padding should be equal so let's use 47px. Since the Td elements have their own padding and it would be annoying to remove their padding each time, just add some extra padding to tr. i.e. px="20px", py="2.5px"

Actually idk if this is that important @sherryhli . If we change the theme to accommodate for this padding then other places where we use a Tr but isn't formatted like this will need to be changed. The default spacing from Chakra also looks fine.

Copy link
Member

@sherryhli sherryhli left a comment

Choose a reason for hiding this comment

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

Nice! 🔥 Some small code improvement suggestions and a minor UI deviation, LGTM otherwise!

@sherryhli
Copy link
Member

padding for the rows is a little off image
I'm pretty sure the horizontal padding should be equal so let's use 47px. Since the Td elements have their own padding and it would be annoying to remove their padding each time, just add some extra padding to tr. i.e. px="20px", py="2.5px"

Actually idk if this is that important @sherryhli . If we change the theme to accommodate for this padding then other places where we use a Tr but isn't formatted like this will need to be changed. The default spacing from Chakra also looks fine.

@LenaNguyen Yeah I'm for staying consistent to a single theme variant if possible, could we check with design to see if this small deviation is okay?

@sherryhli
Copy link
Member

Just realized something: with the way we plan on fetching data for the schedule creation frontend (running a query on the signups table rather than shifts), we wouldn't know about any shifts without signups. That means the row type below would not be seen, and the shift would also not be shown as a dot in the calendar.

row

cc @JoshuaYe @LenaNguyen Do you have any input on the necessity and priority of this feature? How useful is it to view shifts that do not have any signups in schedule creation?

(@lambo-liu no change needed for now, please keep this row type in the code)

@LenaNguyen
Copy link
Member

Just realized something: with the way we plan on fetching data for the schedule creation frontend (running a query on the signups table rather than shifts), we wouldn't know about any shifts without signups. That means the row type below would not be seen, and the shift would also not be shown as a dot in the calendar.

row

cc @JoshuaYe @LenaNguyen Do you have any input on the necessity and priority of this feature? How useful is it to view shifts that do not have any signups in schedule creation?

(@lambo-liu no change needed for now, please keep this row type in the code)

It seems useful to know that there are some shifts that no one is registered for in the overview

@7tint 7tint force-pushed the lambert/schedule-review-table branch from cf42fbf to 2884b4f Compare March 24, 2022 22:48
Copy link
Member

@LenaNguyen LenaNguyen left a comment

Choose a reason for hiding this comment

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

LGTM!

@7tint 7tint merged commit 622925b into main Apr 1, 2022
@7tint 7tint deleted the lambert/schedule-review-table branch April 1, 2022 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schedule review table Schedule review table row
3 participants