Closed rowantran closed 3 years ago
Merging #232 (c43f43a) into main (eea4b8f) will decrease coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #232 +/- ##
============================================
- Coverage 99.81% 99.81% -0.01%
Complexity 342 342
============================================
Files 83 83
Lines 1660 1659 -1
Branches 77 77
============================================
- Hits 1657 1656 -1
Misses 3 3
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
javascript/src/main/pages/Admin/ViewLogins.js | 100.00% <ø> (ø) |
0.00 <0.00> (ø) |
|
...src/main/pages/TutorAssignment/TutorAssignments.js | 100.00% <100.00%> (ø) |
0.00 <0.00> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update eea4b8f...c43f43a. Read the comment docs.
Nice PR! Your PR can be merged once you get an approving review from a staff member and a member of your team.
Keep in mind, code reviews only count if you use the "Approve" or "Request changes" option in GitHub. You can find this under the "Files changed" tab under the "Review changes" button.
Generated by :no_entry_sign: dangerJS against c43f43a231f213635f4c002a83ba3701c1464cd0
These code changes look good, but it looks like the linked issue also specifies that you should add a description to the Login History page. This should just be a short text description about what the info on the page represents- i.e. a list of all the log-ins on the app.
Sorry, the user story and acceptance criteria that I copied from the original issue do seem to imply that, but the implementation todos in #190 suggest that the "description" in question is just the page header. It should be a quick change either way, but do you think an actual text description is still necessary?
These code changes look good, but it looks like the linked issue also specifies that you should add a description to the Login History page. This should just be a short text description about what the info on the page represents- i.e. a list of all the log-ins on the app.
Sorry, the user story and acceptance criteria that I copied from the original issue do seem to imply that, but the implementation todos in #190 suggest that the "description" in question is just the page header. It should be a quick change either way, but do you think an actual text description is still necessary?
Looking at the todos, I think you're right that this should be sufficient
Stored the button on the Tutor Assignments page as a variable to avoid duplicating the markup for it. Also replaced the hardcoded
style
attributes with the appropriate Bootstrap utility classes in the following locations: