[Feature:TAGrading] 3 panel mode #5759

mukul-kmr-jha · 2020-07-31T06:24:26Z

Please check if the PR fulfills these requirements:

The PR title and message follows our guidelines
Tests for the changes have been added/updated (if possible)
Documentation has been updated/added if relevant

What is the current behavior?

Currently, the new TA grading interface only supports a maximum of two panels to be shown.

What is the new behavior?

Introduce the third panel in the ta-grading interface.

Steps to Use/this new panel

Navigate to the ta-grading page
Click on the tags icon in the panel-header to enter new interface
Click on columns (panels) icon to switch between all the panel mode

Other information?

If you previously have opened this new TA grading interface you will need to clear out your localstorage... or paste this localstorage.clear() in your JS console.

codecov · 2020-07-31T06:34:56Z

Codecov Report

Merging #5759 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #5759   +/-   ##
=========================================
  Coverage     22.40%   22.40%           
  Complexity     6231     6231           
=========================================
  Files           157      157           
  Lines         20405    20405           
=========================================
  Hits           4571     4571           
  Misses        15834    15834

Flag	Coverage Δ	Complexity Δ
#autograder	`9.72% <ø> (ø)`	`0.00 <ø> (ø)`
#migrator	`99.10% <ø> (ø)`	`0.00 <ø> (ø)`
#php	`21.43% <ø> (ø)`	`6231.00 <ø> (ø)`
#python_submitty_utils	`82.32% <ø> (ø)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

shailpatels · 2020-08-11T02:21:43Z

site/public/js/ta-grading-v2.js

  isFullScreenMode: false,
  isFullLeftColumnMode: false,
  currentOpenPanel: panelElements[0].str,
  currentTwoPanels: {
-    left: null,
-    right: null,
+    leftTop: null,


Since the values in here are supposed to be strings can you wrap them with quotes so its explicit that these keys are not variables.

shailpatels · 2020-08-11T02:22:09Z

site/public/js/ta-grading-v2.js

    // otherwise just toggle it
-    if (isPanelOpen || !(taLayoutDet.isTwoPanelsEnabled && !isMobileView)) {
+    if (isPanelOpen || +taLayoutDet.numOfPanelsEnabled === 1) {


What is the + doing here?

Because of using LocalStorage for saving/ retrieving the variable, it can be converted to a string. So I used it to convert the type of numOfPanelsEnabled from string to number... it's just the alternative of like number(taLayoutDet.numOfPanelsEnabled)

shailpatels · 2020-08-11T02:22:28Z

site/public/js/ta-grading-v2.js

+  $(".grade-panel .panel-position-cont").change(function() {
+    let panelSpanId = $(this).parent().attr('id');
+    let position = $(this).val();
+    console.log(position);


is this console.log needed?

shailpatels · 2020-08-11T02:24:39Z

site/public/js/ta-grading-v2.js

-  taLayoutDet.panelsBucket.leftSelector = ".two-panel-item.two-panel-left";
-  taLayoutDet.panelsBucket.dragBarSelector = ".two-panel-drag-bar";
+  // taLayoutDet.leftSelector = ".two-panel-item.two-panel-left";
+  // taLayoutDet.dragBarSelector = ".two-panel-drag-bar";


is this still needed or can it be deleted?

shailpatels · 2020-08-11T02:25:00Z

site/public/js/ta-grading-v2.js

  }
  // current open panel will be either left or right panel from two-panel-mode
  // passing forceVisible = true, otherwise this method will just toggle it and it will get hidden
  setPanelsVisibilities(taLayoutDet.currentOpenPanel, true);
-  initializeTwoPanelDrag();
+  // initializeTwoPanelDrag();


same comment as above

shailpatels · 2020-08-11T02:31:24Z

site/public/js/ta-grading-v2.js

+  }
+  else {
+    // taLayoutDet.numOfPanelsEnabled is 1
+    alert("Exchange works only when there are two panels...");


I don't think this error message makes sense in this context, something like "Cannot swap left and right panels outside of two pannel mode" would be more descriptive

shailpatels · 2020-08-11T02:32:55Z

site/app/templates/grading/electronic/NavigationBarV2.twig

@@ -63,7 +63,7 @@
                <button title="Toggle the two panel mode"
                        class="invisible-btn key_to_click"


native HTML buttons will trigger the onclick even if they are clicked on with a mouse or with the keyboard, the key_to_click class is not required in this case

shailpatels · 2020-08-11T02:35:40Z

site/public/js/ta-grading-v2.js

+    if (panelSpanId) {
+      const panelId = panelSpanId.split('_btn')[0];
+      setPanelsVisibilities(panelId, null, position);
+      $('select#' + panelId + '_select').hide();


Submitty CSS Id's should use dashes instead of underscores
https://submitty.org/developer/coding_style_guide/css

However, when I was implementing the notebook code I started using dashes before I realized how much of the logic depended on the specific naming scheme. If it's too difficult to remove for now it can be refactored/made a future issue later.

I began modifying some of the logic to use - instead on #5681 so there will be some overlap and merge conflicts most likely

Its actually depended on _ for the DOM manipulation and is also used in the tests the same way... also I'm working on some other issue for TA-grading page, so, I will refactor(- over _) these in that PR.
Let's not worry about the merge conflicts I will update the logics which you are mentioning, thanks:)

shailpatels · 2020-08-11T02:37:45Z

site/public/js/ta-grading-v2.js

-  const panelCont = leftPanel.parentElement;
-  const dragbar = document.querySelector(taLayoutDet.panelsBucket.dragBarSelector);
+  const leftPanel = document.querySelector(taLayoutDet.leftSelector);
+  // const rightPanel = document.querySelector(taLayoutDet.panelsBucket.rightSelector);


is this needed still?

shailpatels · 2020-08-11T02:37:54Z

site/public/js/ta-grading-v2.js

+      ? taLayoutDet.panelsBucket.rightBottomSelector
+      : taLayoutDet.panelsBucket.leftBottomSelector
+  );
+  // const rightPanel = document.querySelector(taLayoutDet.panelsBucket.rightSelector);


same comment as above

elihschiff · 2020-08-17T16:09:48Z

Just a small comment. When you hover over the vertical blue bar between the 2 panels it fades/blurs however when you hover over the horizontal blue bar it does not. I think it should be consistent.

Screenshot taken when my mouse is over the blue bar even though it does not show up in screenshot

Same here but it does not blur while the other one does

…rading

bmcutler

Works pretty well. I think there are some small bugs with the horizontal slider in 3 panel mode, when combined with full height left and/or hide sidebar & topbar. But I think this will be revised as you switch to the panel layout modal, so I will merge this now :)

bmcutler requested a review from elihschiff August 5, 2020 05:14

emaicus self-requested a review August 7, 2020 17:46

shailpatels requested changes Aug 11, 2020

View reviewed changes

mukul-kmr-jha added 18 commits August 11, 2020 19:48

refactor full-left col DOM manipulation

3899847

add third column

e91d910

handle next panel case and add dragbar

088115d

switching screen not working properly

7741c69

integrating dragging functionality in top-down rows

ecc9454

fix only top left(half-screen) showing

5c4dfa4

full-left-col not working

aad6145

update panel-selection-options whenever layout changes

db7492f

add split rows on right side also

3d8f2f0

fix typo

b58d4fa

fix srollbar styles

7c6c989

integrate exchange panels in 3 panels-mode

15fc5ab

persist the changes on page refresh

45262ae

fixing dropdown options on page-refresh

95e7ad8

minor cleanup

a21de92

refactor and create drag-resize util file

144e417

major cleanups

9adab6c

add third column

7dc0848

mukul-kmr-jha force-pushed the 3-panel-mode-ta-grading branch from 23d1ef5 to 7dc0848 Compare August 11, 2020 19:22

mukul-kmr-jha mentioned this pull request Aug 17, 2020

[Feature:TAGrading] layout selector modal #5831

Merged

3 tasks

mukul-kmr-jha and others added 4 commits August 18, 2020 20:36

Merge remote-tracking branch 'upstream/master' into 3-panel-mode-ta-g…

c80566a

…rading

hover on drag-bar and linting

9976f06

Merge branch 'master' into 3-panel-mode-ta-grading

158eb37

Merge branch 'master' into 3-panel-mode-ta-grading

7660d6e

bmcutler self-requested a review August 20, 2020 05:36

bmcutler reviewed Aug 20, 2020

View reviewed changes

bmcutler changed the title ~~[Feature: InstructorUI] Introduce third panel mode in the ta-grading page~~ [Feature:TAGrading] 3 panel mode Aug 20, 2020

bmcutler merged commit 84e930c into master Aug 20, 2020

bmcutler deleted the 3-panel-mode-ta-grading branch August 20, 2020 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Feature:TAGrading] 3 panel mode #5759

[Feature:TAGrading] 3 panel mode #5759

Uh oh!

mukul-kmr-jha commented Jul 31, 2020

Uh oh!

codecov bot commented Jul 31, 2020 •

edited

Loading

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

mukul-kmr-jha Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

mukul-kmr-jha Aug 11, 2020 •

edited

Loading

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

shailpatels Aug 11, 2020

Uh oh!

elihschiff commented Aug 17, 2020

Uh oh!

bmcutler left a comment

Uh oh!

Uh oh!

		@@ -63,7 +63,7 @@
		<button title="Toggle the two panel mode"
		class="invisible-btn key_to_click"

[Feature:TAGrading] 3 panel mode #5759

[Feature:TAGrading] 3 panel mode #5759

Uh oh!

Conversation

mukul-kmr-jha commented Jul 31, 2020

Please check if the PR fulfills these requirements:

What is the current behavior?

What is the new behavior?

Steps to Use/this new panel

Other information?

Uh oh!

codecov bot commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukul-kmr-jha Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elihschiff commented Aug 17, 2020

Uh oh!

bmcutler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jul 31, 2020 •

edited

Loading

mukul-kmr-jha Aug 11, 2020 •

edited

Loading