Employee Modal Update Image #487
Conversation
|
[diff-counting] Significant lines: 320. |
| @@ -128,7 +128,7 @@ const EmployeeCard = ({ | |||
| firstName={firstName} | |||
| lastName={lastName} | |||
| netId={netId} | |||
| photoLink={photoLink} | |||
| photoLink={photoLink ? photoLink + '?v=' + new Date().getTime() : ''} | |||
There was a problem hiding this comment.
tiny thing but I think using string interpolation could make the string manipulation clearer
| * `days` properties, and returns an object where each day is a key and the value is an object | ||
| * containing the `startTime` and `endTime` for that day. | ||
| * @param {ObjectType[]} availability - The `availability` parameter is an array of objects. Each | ||
| * object in the array represents a time slot with the following properties: |
There was a problem hiding this comment.
I think this line is cut off short (I don't see any following properties)
| * @param {string} employeeId - The employeeId parameter is a string that represents the unique | ||
| * identifier of the employee for whom the photo is being uploaded. |
There was a problem hiding this comment.
This is more of a general comment but I think we can and should be less verbose when writing documentation like this.
For example @param {string} employeeId - unique identifier of the employee for whom the photo is being uploaded is shorter but provides the same information
There was a problem hiding this comment.
Even the {string} doesn't need to be there tbh, the type is included in the code already.
Kinda going off Google's style guide
| @@ -225,57 +296,141 @@ const EmployeeModal = ({ | |||
| if (selectedRole.includes('driver')) { | |||
| if (selectedRole.some((role) => role.includes('admin'))) { | |||
| if (existingDriver && existingAdmin) { | |||
| await createOrUpdateDriver(driver, false); | |||
| await createOrUpdateAdmin(admin, false); | |||
| await createOrUpdateEmployee( | |||
There was a problem hiding this comment.
This is a lot of calls to createOrUpdateEmployee with basically two sets of params. One way to make this more concise imo is to create two helper functions for drivers and admins. It looks like the last three params are always the same for each scenario
| <p>{trunc ? truncate(body) : body}</p> | ||
| </div> | ||
| <div className={styles.link} onClick={onViewClick}> | ||
| {trunc ? 'View More' : 'Less'} |
There was a problem hiding this comment.
I don't think this behavior makes sense if the body doesn't need to be truncated (ie there's nothing to "View More")
|
|
||
| type Message = { | ||
| key: number; |
There was a problem hiding this comment.
If key is just going to be the index of the Message in the newMessages array, I don't think we need it.
| photoLink={employee.photoLink} | ||
| photoLink={ | ||
| employee.photoLink | ||
| ? employee.photoLink + '?v=' + new Date().getTime() |
There was a problem hiding this comment.
same comment as above - also this totally could go into a helper function because it's very specific behavior that if updated needs to be updated everywhere
|
I pulled and tested this PR and the Employee image does load immediate without having to refresh. Great PR, works as intended |
Summary
This pull request is the first step towards improving the user experience when editing or creating an employee. While working on the task, I found about a bug where editing an employee's image was not working properly because of some inconsistency. I resolved all the issues with editing employee images.
Along with that, I also made sure the images were updated on the client side without the need to refresh.
Issue: When updating the user image, the URL remains the same. Therefore, react does not refresh the image.
Solution: I changed the URL by adding the current time. That way the image URL is always different and react will render the right image after an employee has been updated without needing to refresh.
I also improved the code quality and added documentation to one of the most complicated functionalities on the project: adding and editing an employee. Because the same modal is used to edit, create, promote, demote, and delete an employee (admin or driver), the component's logic is highly complicated. I broke down and combined some functions to improve readability as well as added JSDocs.
Test Plan