Skip to content
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

[Manage Submissions] Score popup modal styling + fixes #1952

Open
wants to merge 6 commits into
base: feature/new-manage-submissions
Choose a base branch
from

Conversation

lykimchee
Copy link
Member

@lykimchee lykimchee commented Jul 24, 2023

Summary

Summary generated by Reviewpad on 20 Sep 23 17:22 UTC

This pull request includes three patches:

  • Patch 1/3: Style fixes. This patch modifies the JavaScript and CSS files related to managing submissions. It includes changes to improve the styling and formatting of the submission details table.
  • Patch 2/3: Fix formatting and styling. This patch further improves the formatting and styling of the submission details table in the JavaScript and CSS files.
  • Patch 3/3: Fix undefined error showing up for all problems and scores. This patch fixes an issue where an "undefined" error was being displayed for all problems and scores in the submission details table.

Description

Adds further styling and bug fixes to the score popup modal, mainly those noted from this comment.

Motivation and Context

Part of the Manage Submissions UI overhaul.

How Has This Been Tested?

Style-related issues

  1. The “X” button (to close the popup) positioning

Old:
image

New:
image

  1. The score under each problem has been fixed (no longer bold, still italicized) to follow the Figma prototype

  2. Disabled the table rows changing color (to blue) on mouseover, for both the manage submissions main table and the table in the score popup

  3. Changed the background color for problem columns to follow the Figma prototype

Old:
image

New:
image

  1. Added sort icons to header items in score details table

Old:
image

New:
image

Bug Fixes

  1. Error for auto graded assignment where problem scores don't exist (e.g. created a submission, autograde failed, etc.) => result in error while mapping problem data into the tables Note for reviewer: Currently made this case say undefined in the table popup to users, but the word "undefined" doesn't feel very user-friendly, so would appreciate any suggestions for a substitute!

Old:
(for incorrect file upload)
image
(for excused students)
image

New:
image

  1. Problem max score should have “(Autograded)” after the score if an autograded problem. Note: Would be nice if tester could set up an assignment with a mix of autograded + not autograded problems if possible, because I think there might be an issue with the way I implemented this— I was originally planning to base this on grader_id per problem, but grader_id is in data.scores while problems + scores came from data.submissions, so I used data.autograded to keep it simple...

Old:
image

New:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@lykimchee lykimchee requested a review from damianhxy July 24, 2023 05:08
@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review labels Sep 20, 2023
${problem.name}
<br>
<i> ${max_score} ${autograded} </i>
const autograded = data.autograded ? " (Autograded)" : "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2023-10-06 at 02 11 21

Seems to erroneously mark non-autograded problems as autograded

Copy link
Member

@damianhxy damianhxy Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that autograded is set via autograded = @assessment.has_autograder?, so it's not a good way to check if a problem is autograded

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it makes sense for it to be a column variable and am not sure about the usefulness of (Autograded). Will remove

@damianhxy
Copy link
Member

Screenshot 2023-10-06 at 02 17 40

Unrelated to this PR, but the wrong tags were being used here in submissions/index.html.erb: you should add external_

@@ -44,14 +44,20 @@ $(document).ready(function () {

// Fetch data and render it in the modal
get_score_details(course_user_datum_id).then((data) => {
const sorting_icons =
Copy link
Member

@damianhxy damianhxy Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2023-10-06 at 02 29 08

Note that once datatables-rows is correctly included, there are existing sorting icons, so there is actually no need to implement your own

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I personally think we should remove the link to Datatables stylesheet and use the styling/logic Lynn implemented because:

  1. The default Datatables styling results in the icon appearing on all columns and sorting a column like "Actions" is weird. Disabling the icons is not straightforward: https://datatables.net/forums/discussion/41214/how-to-leave-column-sorting-enabled-but-remove-the-sorting-icons
  2. The icons that Lynn added looks better IMO and changing the original icons to use the materialize icons would require the same amount of code.

Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, but left some comments

@damianhxy damianhxy marked this pull request as draft December 28, 2023 22:34
@KesterTan KesterTan self-assigned this Nov 14, 2024
@KesterTan
Copy link
Contributor

Also edited the margins so that the icons are centered

Screenshot 2024-11-17 at 2 37 11 PM

@KesterTan KesterTan marked this pull request as ready for review November 17, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge medium Pull request is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants