Fix in-video quiz question issues with display, sizing, and alignment #11

karandikar · 2017-05-02T19:05:46Z

This set of commits fixes issues with the question alignment when the video isn't the first component, question sizing/alignment when a video is resized (window resize or un-fullscreening) with a question open, and questions displaying inconsistently because of a css issue.

karandikar · 2017-05-02T19:08:46Z

@stvstnfrd @caesar2164 Let me know if any changes need to be made here. These changes solve the problem, but I'm aware they're inelegant/inefficient; I'll continue looking for an event I could listen for to detect a fullscreen change instead of checking every 200 milliseconds.

coveralls · 2017-05-02T19:09:16Z

Changes Unknown when pulling 438be6b on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

caesar2164 · 2017-05-08T22:37:24Z

invideoquiz/public/js/src/invideoquiz.js

+        knownVideoPosition = $('.tc-wrapper', video).position().top;
+        knownVideoHeight = $('.tc-wrapper', video).css('height');
+        knownVideoWidth = $('.tc-wrapper', video).css('width');
+    }


function getVideoDimensions() { var videoPosition = $('.tc-wrapper', video).position().top; var videoHeight = $('.tc-wrapper', video).css('height'); var videoWidth = $('.tc-wrapper', video).css('width'); return [videoPosition, videoHeight, videoWidth]; } var knownVideoDimensions = getVideoDimensions();

caesar2164 · 2017-05-08T22:38:11Z

invideoquiz/public/js/src/invideoquiz.js

+              // both our known size measurements and the size of the problem
+              currentVideoPosition = $('.tc-wrapper', video).position().top;
+              currentVideoHeight = $('.tc-wrapper', video).css('height');
+              currentVideoWidth = $('.tc-wrapper', video).css('width');


define these with the function commented above!

karandikar · 2017-05-08T23:54:34Z

@caesar2164 Fixed, let me know if anything else needs to change!

caesar2164 · 2017-05-09T00:05:03Z

invideoquiz/public/js/src/invideoquiz.js

@@ -25,6 +31,7 @@ function InVideoQuizXBlock(runtime, element) {

            if (studentMode) {
                setUpStudentView(component);
+                setUpVideoDimensions();


let's just make this:

knownVideoDimensions = getVideoDimensions();

caesar2164 · 2017-05-09T00:05:17Z

invideoquiz/public/js/src/invideoquiz.js

+
+    function setUpVideoDimensions() {
+        knownVideoDimensions = getVideoDimensions();
+    }


let's delete this function

caesar2164 · 2017-05-09T00:08:15Z

invideoquiz/public/js/src/invideoquiz.js

+              if ((currentVideoDimensions[0] !== knownVideoDimensions[0]) ||
+                  (currentVideoDimensions[1] !== knownVideoDimensions[1]) ||
+                  (currentVideoDimensions[2] !== knownVideoDimensions[2])) {
+                    resizeInVideoProblem(problemToDisplay, video);


let's pass currentVideoDimensions as the third value to this function

ie:

resizeInVideoProblem(problemToDisplay, video, currentVideoDimensions);

rewrite:

resizeInVideoProblem(problemToDisplay, currentVideoDimensions);

caesar2164 · 2017-05-09T00:08:46Z

invideoquiz/public/js/src/invideoquiz.js

@@ -61,14 +79,22 @@ function InVideoQuizXBlock(runtime, element) {
            }
        });
    }
+
+    function resizeInVideoProblem(currentProblem, currentVideo) {
+        videoDimensions = getVideoDimensions();


let's have this function take a third var for the videoDimensions

ie:

function resizeInVideoProblem(currentProblem, currentVideo, videoDimensions) {

and then we can just delete this line.

Actually why do we even pass currentVideo?

better rewrite:

function resizeInVideoProblem(currentProblem, videoDimensions) {

caesar2164 · 2017-05-09T00:17:45Z

invideoquiz/public/js/src/invideoquiz.js

+
+    function resizeInVideoProblem(currentProblem, currentVideo) {
+        videoDimensions = getVideoDimensions();
+        currentProblem.css({top: videoDimensions[0], height: videoDimensions[1], width: videoDimensions[2]});


just a quick readability cleanup:

currentProblem.css({ top: videoDimensions[0], height: videoDimensions[1], width: videoDimensions[2] });

caesar2164 · 2017-05-09T00:21:52Z

invideoquiz/public/js/src/invideoquiz.js

@@ -87,7 +113,9 @@ function InVideoQuizXBlock(runtime, element) {
                if (isProblemToDisplay) {
                  problemToDisplay = $('.xblock-student_view', this)
                  videoState.videoPlayer.pause();
+                  resizeInVideoProblem(problemToDisplay, video);


as a consequence of changing the resizeInVideoProblem function let's make this read:

resizeInVideoProblem(problemToDisplay, video, knownVideoDimensions);

rewrite:

resizeInVideoProblem(problemToDisplay, knownVideoDimensions);

coveralls · 2017-05-09T16:24:56Z

Changes Unknown when pulling d8c4310 on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

karandikar · 2017-05-10T21:36:18Z

@caesar2164 Made the necessary edits! Let me know what you think

coveralls · 2017-05-10T21:36:24Z

Changes Unknown when pulling d191939 on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

caesar2164 · 2017-05-10T21:49:33Z

invideoquiz/public/js/src/invideoquiz.js

+        videoWidth = $('.tc-wrapper', video).css('width');
+        return [videoPosition, videoHeight, videoWidth];
+    }
+


let's add a videoDimensionsDiffer() function like so:

function videoDimensionsDiffer(currentVideoDimensions, knownVideoDimensions) { if (currentVideoDimensions.length !== knownVideoDimensions.length) { return true; } for (var i = 0, i < knownVideoDimensions.length, i++) { if (knownVideoDimensions[i] !== currentVideoDimensions[i]) { return true; } } return false; }

@caesar2164 done!

caesar2164 · 2017-05-10T21:50:34Z

invideoquiz/public/js/src/invideoquiz.js

+              // both our known size measurements and the size of the problem
+              currentVideoDimensions = getVideoDimensions();
+
+              if ((currentVideoDimensions[0] !== knownVideoDimensions[0]) ||


this conditional can now be:

if (videoDimensionsDiffer(currentVideoDimensions, knownVideoDimensions)) { ... }

coveralls · 2017-05-10T22:09:04Z

Changes Unknown when pulling 55976e2 on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

caesar2164 · 2017-05-10T22:17:04Z

invideoquiz/public/js/src/invideoquiz.js

+    // Interval at which to check if video size has changed size
+    // and the displayed problems needs to do the same
+    var resizeIntervalTime = 100;
+
    // Interval at which to check for problems to display
    // Checking every 0.5 seconds to make sure we check at least once per actual second of video
    var intervalTime = 500;


lets rename this variable, now that we have multiple intervals...

caesar2164 · 2017-05-10T22:18:47Z

invideoquiz/public/js/src/invideoquiz.js

+            resizeIntervalObject = setInterval(function () {
+
+              // check if the size has changed from the previous state; if so, update
+              // both our known size measurements and the size of the problem


this comment can probably go away now that the code is easier to read...

coveralls · 2017-05-10T22:37:43Z

Changes Unknown when pulling e95f268 on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

caesar2164 · 2017-05-10T22:40:29Z

invideoquiz/public/js/src/invideoquiz.js

@@ -50,6 +57,25 @@ function InVideoQuizXBlock(runtime, element) {
        }


Doh, I missed this before, lets move this to here, so it only gets done once...

if (studentMode) { knownVideoDimensions = getVideoDimensions(); }

caesar2164 · 2017-05-10T22:40:49Z

invideoquiz/public/js/src/invideoquiz.js


    $(function () {
        $('#seq_content .vert-mod .vert').each(function () {
            var component = $(this);

            if (studentMode) {
                setUpStudentView(component);
+                knownVideoDimensions = getVideoDimensions();


move down to location of other comment.

coveralls · 2017-05-10T22:47:38Z

Changes Unknown when pulling 04fa075 on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

caesar2164 · 2017-05-11T18:02:16Z

invideoquiz/public/js/src/invideoquiz.js

+        return [videoPosition, videoHeight, videoWidth];
+    }
+
+    function videoDimensionsDiffer(newMeasurement, oldMeasurement) {


looks great!

caesar2164 · 2017-05-11T18:02:45Z

invideoquiz/public/js/src/invideoquiz.js

@@ -31,6 +37,7 @@ function InVideoQuizXBlock(runtime, element) {
        });

        if (studentMode) {
+          knownVideoDimensions = getVideoDimensions();


awesomesauce

stvstnfrd · 2017-05-11T18:49:04Z

invideoquiz/public/js/src/invideoquiz.js

@@ -50,6 +57,25 @@ function InVideoQuizXBlock(runtime, element) {
        }
    }

+    function getVideoDimensions() {
+        videoPosition = $('.tc-wrapper', video).position().top;


This variable needs declared first.

stvstnfrd · 2017-05-11T18:49:07Z

invideoquiz/public/js/src/invideoquiz.js

@@ -50,6 +57,25 @@ function InVideoQuizXBlock(runtime, element) {
        }
    }

+    function getVideoDimensions() {
+        videoPosition = $('.tc-wrapper', video).position().top;
+        videoHeight = $('.tc-wrapper', video).css('height');


This variable needs declared first.

stvstnfrd · 2017-05-11T18:49:11Z

invideoquiz/public/js/src/invideoquiz.js

+    function getVideoDimensions() {
+        videoPosition = $('.tc-wrapper', video).position().top;
+        videoHeight = $('.tc-wrapper', video).css('height');
+        videoWidth = $('.tc-wrapper', video).css('width');


This variable needs declared first.

Yep! My bad

stvstnfrd · 2017-05-11T18:49:59Z

invideoquiz/public/js/src/invideoquiz.js

+        videoPosition = $('.tc-wrapper', video).position().top;
+        videoHeight = $('.tc-wrapper', video).css('height');
+        videoWidth = $('.tc-wrapper', video).css('width');
+        return [videoPosition, videoHeight, videoWidth];


videoPosition is an unexpected value to be returned from getVideoDimensions;
is there a more apt name we can use?

Also, looking at how this data is actually used, I suspect a dictionary/object may be a more apt data structure.

return { 'top': position, 'height': height, 'width': width };

That means when we're modifying the CSS below, it's as easy as:

currentProblem.css(videoDimensions);

Of course, the for loop will need updated, but that should be simple enough:

for (var key in knownVideoDimensions) { if (knownVideoDimensions.hasOwnProperty(key)) { if (knownVideoDimensions[key] !== currentVideoDimensions[key]) { return true; } } } return false;

This makes the code more obvious, by avoiding directly indexing into the array arr[0], arr[1], etc., as that's opaque.

This is a great idea, implemented it

stvstnfrd · 2017-05-11T18:54:24Z

invideoquiz/public/js/src/invideoquiz.js

+    }
+
+    function videoDimensionsDiffer(newMeasurement, oldMeasurement) {
+        if (newMeasurement.length !== oldMeasurement.length) {


When would this case occur?
Seems like only on the first run, because oldMeasurement (via knownVideoDimensions) is not initialized when being declared; does that seem right?

If so, could we just initialize it to some value above and drop this special handling here?

var knownVideoDimensions = [0, 0, 0];

This case would never occur, since knownVideoDimensions is set right at the outset. Regardless, it's now unnecessary with the changes you've suggested since!

stvstnfrd · 2017-05-11T18:58:50Z

invideoquiz/public/js/src/invideoquiz.js

        });

        video.on('pause', function () {
          clearInterval(intervalObject);
          if (problemToDisplay) {
+            resizeIntervalObject = setInterval(function () {
+              currentVideoDimensions = getVideoDimensions();


Should this be declared as a local variable?

stvstnfrd · 2017-05-11T19:11:58Z

invideoquiz/public/js/src/invideoquiz.js

@@ -50,6 +57,25 @@ function InVideoQuizXBlock(runtime, element) {
        }
    }

+    function getVideoDimensions() {
+        videoPosition = $('.tc-wrapper', video).position().top;


Nitpick: I tend to avoid prefixing variables if we're doing it for everything.
From the context of this function getVideoDimensions, it's obvious that
any dimensions we list are for videos, unless otherwise noted.

Not only does it cut down on some verbosity, but it also makes it easier to grep the code: grepping for position (case sensitive) won't find videoPosition, but it will find position.

Yeah, this is fair. Made the requested changes

karandikar · 2017-05-12T23:44:13Z

@stvstnfrd Made a bunch of changes based on your recommendations, let me know what you think!

coveralls · 2017-05-12T23:45:24Z

Changes Unknown when pulling e56e2aa on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

stvstnfrd · 2017-05-15T18:49:08Z

+1 lgtm, thanks for implementing those changes!

caesar2164 · 2017-05-15T18:51:45Z

@karandikar awesome, time to squash commits!

If a video isn't the first component in the unit, the in-video-quiz questions appear incorrectly positioned on the page. We reposition the questions to properly cover the video while it's paused.

Previously, questions would sometimes fail to display in the in-video quiz xblocks. This was likely due to the "display" css property not changing from "none" to "block" for the problem component. This would also trigger issues with the videoState variable as well.

Previously, if the video transitioned from fullscreen to regular size with a question open, or if the window was resized with a question open, the question would not resize properly (the position, height, and/or width would be incorrect). This commit checks for changes to the video dimensions every 100ms when a question is open, and resizes the question if a change is detected.

- Rename ambiguous interval and timeout variables - Rename functions to reduce verbosity - Switch data structure for video dimension to be dictionary instead of list - Remove unnecessary comment

karandikar · 2017-05-16T19:10:49Z

@caesar2164 @stvstnfrd Squashed, take a look and let me know if anything needs changing!

coveralls · 2017-05-16T19:11:57Z

Changes Unknown when pulling 8abbbd6 on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

coveralls · 2017-05-16T19:29:03Z

Changes Unknown when pulling 20d2a26 on karandikar:fix-in-video-quiz into ** on Stanford-Online:master**.

caesar2164 reviewed May 8, 2017

View reviewed changes

caesar2164 reviewed May 9, 2017

View reviewed changes

karandikar force-pushed the fix-in-video-quiz branch from d8c4310 to d191939 Compare May 10, 2017 21:34

caesar2164 reviewed May 10, 2017

View reviewed changes

caesar2164 approved these changes May 11, 2017

View reviewed changes

stvstnfrd requested changes May 11, 2017

View reviewed changes

stvstnfrd approved these changes May 15, 2017

View reviewed changes

karandikar added 4 commits May 16, 2017 11:59

Fix in-video quiz question misalignment when video isn't first component

372c75c

If a video isn't the first component in the unit, the in-video-quiz questions appear incorrectly positioned on the page. We reposition the questions to properly cover the video while it's paused.

Fix stylistic issues with javascript

8abbbd6

- Rename ambiguous interval and timeout variables - Rename functions to reduce verbosity - Switch data structure for video dimension to be dictionary instead of list - Remove unnecessary comment

karandikar force-pushed the fix-in-video-quiz branch from e56e2aa to 8abbbd6 Compare May 16, 2017 19:10

caesar2164 approved these changes May 16, 2017

View reviewed changes

Bump release patch version from 0.1.4 to 0.1.5

20d2a26

caesar2164 merged commit dce4d5f into Stanford-Online:master May 16, 2017

		@@ -50,6 +57,25 @@ function InVideoQuizXBlock(runtime, element) {
		}

Fix in-video quiz question issues with display, sizing, and alignment #11

Fix in-video quiz question issues with display, sizing, and alignment #11

Conversation

karandikar commented May 2, 2017

karandikar commented May 2, 2017

coveralls commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karandikar commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar2164 May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar2164 May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 9, 2017

karandikar commented May 10, 2017

coveralls commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar2164 May 10, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karandikar commented May 12, 2017

coveralls commented May 12, 2017

stvstnfrd commented May 15, 2017

caesar2164 commented May 15, 2017

karandikar commented May 16, 2017

coveralls commented May 16, 2017

coveralls commented May 16, 2017

caesar2164 May 9, 2017 •

edited

Loading

caesar2164 May 9, 2017 •

edited

Loading

caesar2164 May 10, 2017 •

edited

Loading