Student admin management #404
Conversation
MillerSenior
force-pushed
the
student_admin_management
branch
from
e6a6229 to
4520e21
Compare
|
Updated student and admin management pages to accomodate figma closes #395 |
|
For some reason the trash can icon isn't displaying in the code but is in the figma on the admin page. Is this element needing a condition to render, also, the forms don't submit and I'd like to be able to add more sers to test the view further. |
jtucholski
force-pushed
the
student_admin_management
branch
from
0f99a5d to
b415708
Compare
There was a problem hiding this comment.
@MillerSenior I left a few comments on both files for things that stood out to me on the initial review. That's not to say there might not be more, but before we had Carolina review it I wanted to have you look into these.
|
Closes #395 once it is merged. |
There was a problem hiding this comment.
Overall things to note:
- This page refactor is pretty similar to the setup on
ProviderPlatformManagement.tsx, so you can use that page as "code inspiration" as the code should look fairly similar. - I left a comment on PR HeroIcon refactor - ResourceManagement.tsx #402, but the icons need to be updated on this page too. I would consult with @corypride to see whether you will handle those updates or he will.
- When developing, please make sure to check dark mode too! If you look at this PR now on dark mode, you can see that the colors are not changing correspondingly. This is why (as Josh mentioned before) we try to only use the colors listed on
tailwind.config.js
I might have only commented on one of the two files, but all comments PROBABLY apply to both files (for example, both files should have a "solid" PlusIcon on the "Add" button, but I only left a comment on one of the files). Please modify both!
| <table className="table-2 w-full border-separate border-spacing-y-4"> | ||
| <thead> | ||
| <tr className="border-gray-600"> | ||
| <th className="flex"> | ||
| <span>Name</span> | ||
| <tr> | ||
| <th className="px-6 pb-2 text-left text-sm font-medium text-gray-1"> | ||
| Name | ||
| </th> | ||
| <th className="px-6 pb-2 text-left text-sm font-medium text-gray-1"> | ||
| Username | ||
| </th> | ||
| <th className="px-6 pb-2 text-left text-sm font-medium text-gray-1"> | ||
| Last Active | ||
| </th> | ||
| <th className="px-6 pb-2 text-left text-sm font-medium text-gray-1"> | ||
| Actions |
There was a problem hiding this comment.
I don't think you'll need to have extra classes here, they should be pre-set if you use table-2. A similar table was implemented on ProviderPlatformManagement.tsx around line 153. It should look fairly similar to that.
| const updatedAt = new Date(user.updated_at); | ||
| return ( | ||
| <tr | ||
| key={user.id} | ||
| className="border-gray-600" | ||
| className="bg-white shadow-sm rounded-lg" |
There was a problem hiding this comment.
Similar thing here. You should use the class card and it should automatically apply the necessary classes for this type of card!
There was a problem hiding this comment.
It also needs to maintain the type there
| <PlusIcon className="h-4 border border-white rounded-full p-1 mr-2" />{' '} | ||
| Add Student |
There was a problem hiding this comment.
You won't need to add these classes to this icon, and rather than it being imported fromoutline, it should be imported fromsolid (the other icons on this page under "Actions" should be imported from outline, though!).
There was a problem hiding this comment.
Totally my bad - you were right on this of it being outline!
| key={user.id} | ||
| className="border-gray-600" | ||
| className="bg-white shadow-sm" | ||
| > | ||
| <td> | ||
| {user.name_first} {user.name_last} | ||
| <td className="first:rounded-l-lg px-6 py-4"> | ||
| <div className="text-sm text-gray-1"> | ||
| {user.name_first}{' '} | ||
| {user.name_last} | ||
| </div> | ||
| </td> | ||
| <td className="px-6 py-4"> | ||
| <div className="text-sm text-gray-1"> | ||
| {user.username} | ||
| </div> | ||
| </td> | ||
| <td>{user.username}</td> | ||
| <td> | ||
| <td className="px-6 py-4"> |
There was a problem hiding this comment.
Make sure these columns align with the headers! Right now they are slightly off.
Also, just saw this question. Because a user should not be able to delete an admin, we have removed that icon from the page! |
Description of the change
Please provide a brief description of the changes included in this PR.
Screenshot(s)
If the PR includes changes to the UI, please add screenshots or a brief screengrab to demonstrate the changes.
Additional context
Please include any additional context or information that the reviewer may need to understand the PR.