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

Removed deadline warning for non-submittable assignments #1701

Merged
merged 3 commits into from
Jan 30, 2023
Merged

Removed deadline warning for non-submittable assignments #1701

merged 3 commits into from
Jan 30, 2023

Conversation

kushalnl7
Copy link
Contributor

Removed deadline warning for non-submittable assignments

Description

Any submission would have a due datetime as well as a late datetime to submit the assignment. These datetimes are displayed on the submission page. The user also have a option to disable the submissions. Previously, these datetimes were displayed even when the submissions are closed. I changed this such that if the submission is disabled, then instead of the datetime, it shows the attention message to the user that the submission is closed.

Before change -

Screenshot from 2023-01-28 00-02-39

After change -

Screenshot from 2023-01-28 00-00-38

Motivation and Context

The change is in regards to the issue mentioned in #1696.

How Has This Been Tested?

As can be seen in the description section, the screenshots are attached about how a change in submission acceptance can change the output message on the submission page.

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

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.

@michellexliu Functionally, this works. We might want to touch up on the wording / styling before merging though.

@kushalnl7
Copy link
Contributor Author

@michellexliu Thanks for the feedback. I will definitely try to improve them as much as possible. I would also like to have your suggestions over it. Please let me know.

@damianhxy
Copy link
Member

I have the following suggestions:

  • Use the wording: Handins are disabled for this assessment.
  • Instead of using red, apply the class .red-text to be consistent with the other "danger styles"

i.e. <b class="red-text">&nbsp;Handins are disabled for this assessment.</b>

Screenshot 2023-01-29 at 04 37 00

Let me know what you think!

@kushalnl7
Copy link
Contributor Author

kushalnl7 commented Jan 29, 2023

Thanks @damianhxy for the suggestions. I will commit the changes in some time.

@kushalnl7
Copy link
Contributor Author

Hi, Please review the PR now after wording and styling changes. Thanks!

@kushalnl7 kushalnl7 requested review from damianhxy and removed request for michellexliu January 29, 2023 19:34
@kushalnl7
Copy link
Contributor Author

I changed the reviewer by mistake. Sorry for that!

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.

@kushalnl7 I took a closer look and left some further comments, but otherwise lgtm :)

app/views/assessments/show.html.erb Outdated Show resolved Hide resolved
app/views/assessments/show.html.erb Outdated Show resolved Hide resolved
@kushalnl7
Copy link
Contributor Author

Hey @damianhxy, thanks for these comments. I have resolved them.

@damianhxy
Copy link
Member

Looks good to me!

@damianhxy damianhxy merged commit 63181f2 into autolab:master Jan 30, 2023
@kushalnl7
Copy link
Contributor Author

Thanks @damianhxy @michellexliu. I learnt a lot from this code. I would definitely love to contribute more to this repo. Since I am just a beginner as an open source contributor, I would like to have some suggestions from your side!

@damianhxy
Copy link
Member

I would suggest #1695 next as it should be a fairly simple change (scoping styles appropriately). Let me know what you think!

@kushalnl7
Copy link
Contributor Author

Yes, looks great. Will go for this next. Thanks!

@kushalnl7 kushalnl7 deleted the hide-deadline-nonSubmittable-assignments branch January 30, 2023 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide deadline warning for non-submittable assignments
2 participants