Student Modal Styling #318
Conversation
.env.example
Outdated
There was a problem hiding this comment.
Was this file deleted for code cleanup reasons?
There was a problem hiding this comment.
I didn't mean to delete this actually i'll go ahead and reinstate it!
| </Typography> | ||
| <Box className={$Modal.editModalContent}> | ||
| <p id="modal-modal-title" className={$Modal.editModalHeader}> | ||
| Editing {student?.first_name || "Student"}'s Profile |
There was a problem hiding this comment.
Would there be a scenario where the student would be null or undefined, and you could then access the modal?
If not, we could get rid of the hard coded student for that scenario.
There was a problem hiding this comment.
That's a good point - I just left it in as a backup but if it seems unnecessary I can go ahead and take it out!
There was a problem hiding this comment.
I think @KCCPMG mentioned that student first name is a required field, but just going to tag him here for confirmation, in case I misunderstood.
There was a problem hiding this comment.
That's right, in the initial migrations it's specified that a student's first name must not be null, but I personally don't see any harm in leaving the || "Student" in there. Thanks for tagging me here, I somehow didn't even see that my review was requested for this PR!
| width: 100%; | ||
| } | ||
|
|
||
| .modalContent { |
There was a problem hiding this comment.
Is .modalContent used later on? If not, that could be a contender for code cleanup.
There was a problem hiding this comment.
It's used as styling for the other modals in the file (not the edit student one) so I left it in there, but we could maybe change the classnames to make the difference more clear?
There was a problem hiding this comment.
Makes sense! In that case, no worries!
There was a problem hiding this comment.
Looks good! I think there's just the merge conflict to handle and this should be fine. As I moved the window around I noticed that the modal can't be seen or scrolled when the height is less than around 690 pixels, but that seems like an extremely unlikely use case.
|
There was a problem hiding this comment.
This looks good! The logic aspect of having one form handle the IEP edit plus the student edit is a remnant of a prior branch rather than this one, but I think that just putting this check in is a very elegant way of removing the unnecessary (and misleading) fields that come up if a student doesn't yet have an IEP. Since the logic is unchanged, the procedure for editing the IEP is still being called, but looking into it, it seems like there's basically zero cost for that since tRPC will actually batch requests, which I didn't know before.
Good question - I'm honestly not quite sure either, might be good to turn the handling of that (if necessary) into another small PR for code cleanups! |
Updated Modal CSS Module and added some sx styling where needed to have the student modal match the design system mockups.
