-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/add dropdown to update application status #126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some distinct issues that need to be addressed in this page. A lot of them don't have anything to do with your card directly. However, I think that you should definitely spend some more time working on the code that you wrote. You figured out how to brute force it into working, but your use of the useEffect hook is not being properly implemented. The statusUpdateFlag should not be necessary after refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added testing for changing the application status via the dropdown is definitely adequate, but I don't know if I would consider it robust without changing the status at least one more time, if not once for each option. Again, I feel comfortable approving as is, but it could be a bit more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, thanks. At first I was worried that I was going to need to create separate fixture files to test each status, but I realized that I could handle it through a forEach loop instead. All of the status updates made through the dropdown should be tested now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deeper I dive into it, the code for the entire page should probably be refactored. It's not just your code by any means. The biggest issues are a number of redundant state variables, inconsistent data handling (this would be a good place to implement destructuring), missing API call error handling, an excessively generic interface (DataCompile), and an excess of useEffect hooks. This all comes together to make the code hard to read and introduces a number of potential bugs.
It is the useEffect issues specifically that are at the heart of the issues that you have been experiencing. The useEffect hook should be a last resort kind of thing as it triggers a re-render of the DOM, and any state changes nested within a useEffect can risk triggering an infinite loop of re-rendering. In general useEffect should only be used when dealing with API calls on component mount, or to synchronize with external state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback! I understand your concern about useEffect, but I don't necessarily view it as a last resort. I don’t believe it triggers a re-render of the DOM by itself—in my implementation, it only causes a re-render when the user interacts with the calendar or dropdown to change the application date or status.
That said, I didn’t initially intend to rely on useEffect, and I actually tried a few different approaches before using it. At first, I tried calling the handleSubmit function directly from the onChange event handlers. But because of React's asynchronous state updates, the state wasn’t updated in time for handleSubmit to reflect the new values. As a result, the status or dateApplied didn’t have the correct values when handleSubmit was triggered.
I also tried creating separate functions to handle the date and status changes, which then called handleSubmit. But I still ran into the same issue because the state was being updated asynchronously and wasn't ready by the time handleSubmit was invoked.
To work around this, I used useEffect to trigger handleSubmit only after the state had been updated. This ensured that I was submitting the correct, updated state values, rather than outdated ones, without causing redundant re-renders.
I'm definitely open to suggestions if you have other thoughts on how to avoid using useEffect here!
}); | ||
|
||
it("Should make a fetch call when update info button is clicked", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is the same in this file below this point - I just updated the formatting to align with the changes that Devlin made in a previous PR.
Type of Change
Description
This PR adds a dropdown on the job application show page where a user can change an application's status without clicking on the "Edit" button. It also adds a related Cypress test, including a new fixture file.
Request for feedback - A user can edit an application's status either through the edit modal or through the dropdown. I tied the functionality to update the application's status to a useEffect that is triggered when a user changes the status. However, that also caused any updates a user made through the edit modal to be immediately submitted as soon as the status was changed, instead of when the user hit the button to submit all of their updates.
To avoid this issue, I added a statusUpdateFlag variable that only changes from false to true when a user changes the application status via the dropdown. So only when both the status changes and the statusUpdateFlag is true will the status update be submitted immediately. Otherwise, if the user makes changes via the modal, those changes will only be submitted when the user clicks the "Update Info" button in the edit modal.
This seemed like a decent way to get around the issue I was having with the form immediately submitting upon any changes made in the modal, but I would love any thoughts on whether there is a different/better way of handling this. Thanks!
Motivation and Context
Closes Issue #79.
Related Tickets
Screenshots (if appropriate):
Added Test?
Checklist: