Add separate vertical scroll bar to exercise submission inspect #1172

EerikSaksi · 2023-04-04T13:30:00Z

Description

What?

The exercise submission can now be scrolled separately in the submission inspect view.

2023-04-04.16-15-47.mp4

Why?
This makes viewing long submissions easier

How?
Add the same scroll separately button to the submission as well, and create saas rules which applies the sticky scrolling correctly.

Fixes #1111

Testing

Remember to add or update unit tests for new features and changes.

How to test your changes in A-plus
How to test accessibility

What type of test did you run?

Ensuring the sticky scrolling buttons works, and persists in LocalStorage
Did you test the changes in

Chrome
Firefox
This pull request cannot be tested in the browser.

Think of what is affected by these changes and could become broken

Translation

Did you modify or add new strings in the user interface? (Read about how to create translation)

Programming style

Did you follow our style guides?
Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

documents inside the doc directory.
README.md.
Aplus Manual.
Other documentation (mention below which documentation).

Is it Done?

Reviewer has finished the code review
After the review, the developer has made changes accordingly
Customer/Teacher has accepted the implementation of the feature

Clean up your git commit history before submitting the pull request!

ihalaij1

Looks good, but I think the code in assessment.js could be improved by making the duplicated code into a function. See the comment I left.

exercise/static/exercise/assessment.js

markkuriekkinen

Nice, it looks good indeed! I requested a few minor stylistic changes.

We also need to test this more. The implementation does not seem to change anything concerning HTML-formatted grader feedback (when the feedback is not just plain text in <pre>). Need to check what happens then.

markkuriekkinen · 2023-04-12T11:54:23Z

assets/sass/pages/_assessment.scss

+		--sticky-top: 0;
+		flex-grow: 1;
+		.submitted-file.sticky > div {
+			@include sticky-submission-scroll


Suggested change

@include sticky-submission-scroll

@include sticky-submission-scroll;

Using the semicolon in the end seems to be the preferred programming style.
https://sass-lang.com/documentation/at-rules/mixin

markkuriekkinen · 2023-04-12T11:55:44Z

assets/sass/pages/_assessment.scss

-				display: flex;
-				flex-direction: column;
-			}
+			@include sticky-submission-scroll


Suggested change

@include sticky-submission-scroll

@include sticky-submission-scroll;

markkuriekkinen · 2023-04-12T12:01:36Z

exercise/static/exercise/assessment.js

@@ -1,4 +1,37 @@
 $(function () {
+    $.fn.addStickyButton = function (localStorageKey, actionCallback) {
+      // The "skicky" feature is enabled if ResizeObserver is available and there


"skicky" -> "sticky"

markkuriekkinen · 2023-04-12T12:07:25Z

exercise/static/exercise/assessment.js

@@ -1,4 +1,37 @@
 $(function () {
+    $.fn.addStickyButton = function (localStorageKey, actionCallback) {


This line and the function definition are indented by 4 spaces, but they should only be indented one level. In JS, we have used only 2 spaces per level as you can see in the line 35:
$(document).on('aplus:translation-ready', function() {

The function body is of course indented more than the outer function(...) line.

markkuriekkinen · 2023-04-12T12:24:05Z

exercise/static/exercise/assessment.js

@@ -1,4 +1,37 @@
 $(function () {
+    $.fn.addStickyButton = function (localStorageKey, actionCallback) {


If the actionCallback is never used, why does the parameter exist?

PasiSa · 2023-05-18T16:21:30Z

It looks like Eerik has pushed a new version to address the comments. Maybe reviewers can check if their comments have been satisfied (and mark this as approved if so)?

ihalaij1

There seems to be some problems still.

For example, try to submit two long Python files to aplus-manual exercise 6.3.6 Wallet.

After toggling/scrolling/toggling the left-hand side scroll separately button a few times, you will notice that it does not always function the way it should (sometimes it scrolls separately when the checkmark isn't checked etc.).

Additionally, the HTML feedback on the right-hand side is not separately scrollable at all.

EerikSaksi · 2023-06-14T08:58:29Z

This should work now hopefully

EerikSaksi · 2023-06-29T11:59:36Z

The scroll separately button should be found based on parameters and not the text of the button. Maybe this is possible when we feed the extrabuttons parameter to the highlightCode function?

Fixes apluslms#1111

The grader feedback already had a 'Scroll separately' button, and it is now also added for the submitted files. Fixes apluslms#1111 Closes apluslms#1172

PasiSa · 2023-11-15T13:31:32Z

The functionality of this pull request will be covered in an later PR #1286 , that is planned to be merged soon. Therefore closing this pull request now.

The grader feedback already had a 'Scroll separately' button, and it is now also added for the submitted files. Fixes apluslms#1111 Closes apluslms#1172

The grader feedback already had a 'Scroll separately' button, and it is now also added for the submitted files. Fixes #1111 Closes #1172

The grader feedback already had a 'Scroll separately' button, and it is now also added for the submitted files. Fixes apluslms#1111 Closes apluslms#1172

ihalaij1 requested changes Apr 11, 2023

View reviewed changes

exercise/static/exercise/assessment.js Outdated Show resolved Hide resolved

ihalaij1 assigned EerikSaksi Apr 11, 2023

ihalaij1 requested a review from markkuriekkinen April 11, 2023 10:47

markkuriekkinen requested changes Apr 12, 2023

View reviewed changes

EerikSaksi force-pushed the separate-vertical-scrollbar branch 2 times, most recently from 0de5ef0 to 7266b0f Compare May 9, 2023 11:14

PasiSa assigned markkuriekkinen and ihalaij1 and unassigned EerikSaksi May 18, 2023

ihalaij1 requested changes May 22, 2023

View reviewed changes

markkuriekkinen assigned EerikSaksi and unassigned markkuriekkinen and ihalaij1 May 30, 2023

EerikSaksi force-pushed the separate-vertical-scrollbar branch from 7266b0f to 0fc9b97 Compare June 9, 2023 11:18

markkuriekkinen assigned markkuriekkinen and unassigned EerikSaksi Jun 16, 2023

EerikSaksi force-pushed the separate-vertical-scrollbar branch from 0fc9b97 to df038d8 Compare July 5, 2023 09:51

Add separate vertical scroll bar to exercise submission inspect

1af958f

Fixes apluslms#1111

EerikSaksi force-pushed the separate-vertical-scrollbar branch from df038d8 to 1af958f Compare July 5, 2023 09:54

markkuriekkinen mentioned this pull request Oct 17, 2023

Add scroll separately button for submitted files on inspect page #1286

Merged

18 tasks

PasiSa closed this Nov 15, 2023

ihalaij1 added a commit that referenced this pull request Nov 21, 2023

Add scroll separately button for submitted files on inspect page

534e51e

The grader feedback already had a 'Scroll separately' button, and it is now also added for the submitted files. Fixes #1111 Closes #1172

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add separate vertical scroll bar to exercise submission inspect #1172

Add separate vertical scroll bar to exercise submission inspect #1172

EerikSaksi commented Apr 4, 2023

ihalaij1 left a comment

markkuriekkinen left a comment

markkuriekkinen Apr 12, 2023

markkuriekkinen Apr 12, 2023

markkuriekkinen Apr 12, 2023

markkuriekkinen Apr 12, 2023

markkuriekkinen Apr 12, 2023

PasiSa commented May 18, 2023

ihalaij1 left a comment •

edited

Loading

EerikSaksi commented Jun 14, 2023

EerikSaksi commented Jun 29, 2023

PasiSa commented Nov 15, 2023

	@include sticky-submission-scroll
	@include sticky-submission-scroll;

		@@ -1,4 +1,37 @@
		$(function () {
		$.fn.addStickyButton = function (localStorageKey, actionCallback) {

Add separate vertical scroll bar to exercise submission inspect #1172

Add separate vertical scroll bar to exercise submission inspect #1172

Conversation

EerikSaksi commented Apr 4, 2023

Description

Testing

Translation

Programming style

Have you updated the README or other relevant documentation?

Is it Done?

ihalaij1 left a comment

Choose a reason for hiding this comment

markkuriekkinen left a comment

Choose a reason for hiding this comment

markkuriekkinen Apr 12, 2023

Choose a reason for hiding this comment

markkuriekkinen Apr 12, 2023

Choose a reason for hiding this comment

markkuriekkinen Apr 12, 2023

Choose a reason for hiding this comment

markkuriekkinen Apr 12, 2023

Choose a reason for hiding this comment

markkuriekkinen Apr 12, 2023

Choose a reason for hiding this comment

PasiSa commented May 18, 2023

ihalaij1 left a comment • edited Loading

Choose a reason for hiding this comment

EerikSaksi commented Jun 14, 2023

EerikSaksi commented Jun 29, 2023

PasiSa commented Nov 15, 2023

ihalaij1 left a comment •

edited

Loading