-
Notifications
You must be signed in to change notification settings - Fork 93
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
fix: Fix rows counter in the Edit Grade modal window #389
fix: Fix rows counter in the Edit Grade modal window #389
Conversation
Thanks for the pull request, @Lunyachek! This repository is currently maintained by @farhaanbukhsh. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## open-release/quince.master #389 +/- ##
===========================================================
Coverage 94.98% 94.98%
===========================================================
Files 140 140
Lines 1356 1357 +1
Branches 264 264
===========================================================
+ Hits 1288 1289 +1
Misses 60 60
Partials 8 8 ☔ View full report in Codecov by Sentry. |
Hi @farhaanbukhsh! The master for this PR has been merged, so this one should be good-to-go. Thanks! |
@mphilbrick211 sure will merge this after testing :) |
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.
Hey @Lunyachek can you have look at the comment and test it. Thanks a lot for the PR. Let me know if you need more clarification.
const tableData = [ | ||
...data, | ||
{ | ||
adjustedGrade: <AdjustedGradeInput />, | ||
date: formatDateForDisplay(new Date()), | ||
reason: <ReasonInput />, | ||
}, | ||
]; |
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.
should we put a check on the data something like
const tableData = [ | |
...data, | |
{ | |
adjustedGrade: <AdjustedGradeInput />, | |
date: formatDateForDisplay(new Date()), | |
reason: <ReasonInput />, | |
}, | |
]; | |
const tableData = data ? [ | |
...data, | |
{ | |
adjustedGrade: <AdjustedGradeInput />, | |
date: formatDateForDisplay(new Date()), | |
reason: <ReasonInput />, | |
}, | |
] : []; |
While testing I landed up on error when data was undefined and this happened multiple times mostly I guess when the states were getting updated.
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 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.
We should backport 4e9270a
hey @mphilbrick211 I have reviewed the PR and added few suggestion and changes |
Thanks, @farhaanbukhsh! Flagging this for you, @Lunyachek! |
@Lunyachek can you please backport the additional commit so that we can safely merge the PR? |
@Lunyachek friendly ping on this |
@mphilbrick211 Hi! Commit has been backported |
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.
👍
- ✅ I tested this on tutor devstack
- ✅ I read through the code
- ❌ I checked for accessibility issues
- ❌ Includes documentation
706d0ba
into
openedx:open-release/quince.master
thanks @Lunyachek for the effort and persistence. |
This is backport from master - #310
TL;DR - The problem was in the rows counter in the Edit Grades modal window. First digit - number of lines excluding the last line with the form. Second digit - grades data. And our proposal is to include last row with form to common counting
What changed?
FYI: @openedx/content-aurora