Conversation
…g when it is loading
|
Thanks for the review. It looks like it is also a way to enter notes, not just to view them, and this seems intentional. I will talk to Brian and see if it is a bug or a feature. |
I have spoken to Brian, and he said it is also used as an input space, so a more generic wording is appropriate. I have done that. It is no longer "view notes" but merely "notes". There is a question of whether people really use this feature, but we can't just remove it. It will be addressed in future pr's. |
MImran2002
left a comment
There was a problem hiding this comment.
Fix 1: The db_test.py should be uncommitted even though it fixes the Tracy issue I remember talking with Brian about this and not moving forward with those changes.
Fix 2: Update the comment on this function app>static>js>allPendingForms.js>loadOverloadModal because there is no AJAX call and one of the parameter is not in use.
Fix 3: Remove unused parameters or variables like laborStatusFormID in loadReleaseModal.
Fix 4: in allPendingForms.html around line 209 and line 218, calls the loadoverloadModal but the second parameter is never used in the js function so having the parameter inputted in the html side is in valid cause the parameter is never used.
done |
Meatchema
left a comment
There was a problem hiding this comment.
Reposting this comment since the other didn't seem to be flagged as my review:
Both buttons seem to work as intended.
image
However, the filters seem to still be resetting after reloading which makes sense, but I am not sure if that is a concern. Is the concern mainly the buttons functionality, the smoothness in which they function, or how it effects filters?
My answers:
Both buttons show up and work properly. The correct information shows and I don't have to click directly on the letters to make it work.
They both have increase in smoothness of usage (they don't require multiple clicks anymore, though there is still a little bit of a load buffer)
If you use a filter and then make a note or click and use manage the page will reset making it harder to go find the persons information you just edited to see your changes.
Note:
Is there something specific I should be looking for in my review?
I had spoken to brian and the revert when the page reloads is not a bug. I appreciate your comment. No, that is all thanks. |
ojmakinde
left a comment
There was a problem hiding this comment.
Looks great. Just a few small changes and should be ready to merge. Great work.
app/static/js/allPendingForms.js
Outdated
| function loadReleaseModal(formHistoryID, laborStatusFormID) { | ||
| $("#modalRelease").modal("show"); | ||
| function loadReleaseModal(formHistoryID) { | ||
| $("#modalRelease").find('.modal-content').html('<div class="modal-body">Loading...</div>'); |
There was a problem hiding this comment.
Why are we setting the HTML to Loading if we're going to set it to the actual content in the next line? Also I love this fix!
There was a problem hiding this comment.
Good one, I have fixed it now. Currently, it will no longer be in that order; it will set it to loading and then render it, giving it time to display the content from the ajax request.
app/static/js/allPendingForms.js
Outdated
| /* | ||
| This method sends an AJAX call to recieve data used to populate | ||
| the overload modal. | ||
| This method shows the modal and renders is loading when it is loading |
There was a problem hiding this comment.
Can you modify the docstring?
app/static/js/allPendingForms.js
Outdated
| This method shows the modal and renders is loading when it is loading | ||
| */ | ||
|
|
||
| $("#overloadModal").find(".modal-content").html('<div class="modal-body">Loading...</div>'); |
There was a problem hiding this comment.
Also curious about the use of the "Loading..." text here
There was a problem hiding this comment.
The need for the "Loading..." is because the modal content persists from the last time it was opened. This creates a "flash" of the wrong student's data, followed by a self-correction when it is updated.
Through the way that it is handeld we are creating a buffer that is user-friendly (as it gives visual feedback), at the same time, renders the right information.
ojmakinde
left a comment
There was a problem hiding this comment.
LGTM. @BrianRamsay, apparently, the spinner that used to exist on the pending forms modal no longer exists. Did you get rid of it?? Also, we're using a simple "Loading..." text for that transitional state now. Thoughts?









Fixes issue #534
Changes:
The erratic nature of the display for the overlode pending forms and pending release forms. One of the main bugs was the use of preloaded data in the pop-up module when displaying a new request. After a buffer time, the modal would be repopulated with the new request. The other problem was the lack of padding due to the HTML code layering, which required the user to press directly on the text to grant access.
Testing: