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

Add edit submission link when evaluating a submission #5190

Merged
merged 9 commits into from
Dec 19, 2023

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Nov 22, 2023

This pull request adds an edit submission link to to the feedback show page.

image

On smaller screens this results in:
image

The new button now also appears on non owned submissions, replacing the edit button on those pages
image

I also generalized some css about icon buttons in card titles. This also improved the submission index page.
old:
image

new:
image

Closes #2054

@jorg-vr jorg-vr self-assigned this Nov 22, 2023
@jorg-vr jorg-vr added the enhancement A change that isn't substantial enough to be called a feature label Nov 22, 2023
@jorg-vr jorg-vr marked this pull request as ready for review November 22, 2023 12:39
@jorg-vr jorg-vr requested a review from a team as a code owner November 22, 2023 12:39
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team November 22, 2023 12:39
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Nov 22, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Nov 22, 2023
@jorg-vr jorg-vr marked this pull request as draft November 22, 2023 14:57
@jorg-vr jorg-vr marked this pull request as ready for review November 22, 2023 14:57
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Nov 23, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Nov 23, 2023
config/locales/js/nl.yml Outdated Show resolved Hide resolved
app/assets/javascripts/i18n/translations.json Outdated Show resolved Hide resolved
@jorg-vr jorg-vr requested a review from niknetniko November 23, 2023 12:37
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

The functionality looks good, but the buttons can use some margin.

  • In the normal view, the buttons are too close to each other horizontally (if you hover over the new button). On small screens, the same is true vertically.

  • I would also move the breakpoint down 1 tier (to large). This then overflows in Dutch for an evaluation, but I would then hide the word "annotations" (the label next to the toggle buttons) instead.

@@ -17,14 +17,18 @@ import {

const MARKING_CLASS = "marked";

function initAnnotations(submissionId: number, courseId: number, exerciseId: number, userId: number, code: string, questionMode = false): void {
function initAnnotations(submissionId: number, courseId: number, exerciseId: number, userId: number, code: string, userIsStudent: boolean, canSubmitAsOwn: boolean): void {
Copy link
Member

Choose a reason for hiding this comment

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

This is becoming a bit of an anti-pattern. For such long list of options, an object might be better (but I don't know if we already do that in other places).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is only used in a single place, so making it an object would be very artificial, definitely with the rather specific options.

Although a case could be made to group the first options into a submission object. This would be more reusable in the frontend. But I see no reason to refactor that right now.

@jorg-vr jorg-vr requested a review from bmesuere December 5, 2023 13:31
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Dec 19, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Dec 19, 2023
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Dec 19, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Dec 19, 2023
@jorg-vr jorg-vr merged commit ffd59e3 into main Dec 19, 2023
17 checks passed
@jorg-vr jorg-vr deleted the feat/resubmit-from-feedback branch December 19, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add button to edit code and resubmit from evaluation
3 participants