Allow a course to open/close their queues #128
Conversation
…-open-close-button
…-open-close-button
|
very open to style changes, please give input on that. The color on the closed queue is the color used by bootstrap alerts. that can be changed. Also, there is no hover animation on closed queues cards. I can add that back if wanted. edit: since i made a new template for closed queues (the way styling was working, it had to happen to get a different color and loose the hover), I can remove some information from the closed queue cards. I think its important to keep the delete and edit buttons, and to keep it clickable (in order to get to the queue page to reopen the queue) edit2: maybe the "Closed Queues" text could be the same size and the "Or, select a course" ? too many design decisions |
|
There's two high-level design bits that I notice from the screenshots:
Otherwise, this looks amazing!! I'm going to leave this unreviewed for @nwalters512 to dive into the code. |
There was a problem hiding this comment.
Overall, looks great! I think the biggest thing this is missing is real-time updates of open/closed status to clients. Students shouldn't have to reload the page to see that a queue is now closed. This shouldn't be too bad to implement - you can take a look at the files in src/socket/ to see how that's done for questions, and it should be pretty similar for queues as a whole.
src/api/queues.js
Outdated
| res.status(201).send(updatedQueue) | ||
| }) | ||
| ) | ||
|
|
||
| router.patch( |
There was a problem hiding this comment.
If we want to stick to proper HTTP semantics (we already violate them in some places, but) we should really just have a single PATCH endpoint that can handle updating name, location, open/closed status, etc; it just updates any fields that are present in the request.
src/components/ClosedQueueCard.js
Outdated
|
|
||
| import ShowForCourseStaff from './ShowForCourseStaff' | ||
|
|
||
| const ClosedQueueCard = ({ |
There was a problem hiding this comment.
This seems like a lot of code duplication - might be better to just have QueueCard take an open prop?
There was a problem hiding this comment.
I was trying this way earlier, I can do it like this, but if I did, then closed cards would have the hover animation. If thats okay, then I can switch it so they use the same component
There was a problem hiding this comment.
earlier an issue was with the color of the card too, but now that would be an easy fix, so hover is the only issue
There was a problem hiding this comment.
You should be able to fix that by just applying a different set of styles?
There was a problem hiding this comment.
v true, i was trying something overly complicated last night lol
| import { updateQueueStatus } from '../actions/queue' | ||
| import QueueStatusToggle from '../components/QueueStatusToggle' | ||
|
|
||
| function mapStateToProps() { |
There was a problem hiding this comment.
You can just pass null to connect instead of defining an empty function.
| @@ -113,43 +115,29 @@ router.patch( | |||
| [ | |||
| requireCourseStaffForQueue, | |||
| requireQueue, | |||
| check('name').isLength({ min: 1 }), | |||
| check('name') | |||
| .optional({ nullable: true }) | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/pages/queue.js
Outdated
| {!this.props.queue.open && ( | ||
| <Card className="bg-light mb-3"> | ||
| <CardBody className="text-center"> | ||
| This queue is closed. |
There was a problem hiding this comment.
In the spirit of friendly copy, maybe add "Check back later!"
as title says.
here are some pictures:
EDIT fixed spacing and color :
fixes #62