Moodle
  1. Moodle
  2. MDL-35581 META: tasks related to RTL theme fixes before 2.4 freeze (Week 39)
  3. MDL-35249

Grading popup menu, in Assignment grading page, should open up to the left, when in RTL mode

    Details

    • Rank:
      43899

      Description

      Grading popup menu, in Assignment grading page, should open up to the left, when in RTL mode

      For some reason, it gets truncated in smaller browser screen sizes. ( I could not manage to capture it with Bonfire chrome extension)

      btw, STATUS column, should have a wider size (see screen capture)

        Issue Links

          Activity

          Hide
          Nadav Kavalerchik added a comment -

          Simon
          Can have a quick look at this issue and see if you can give a direction i can go into
          for solving this issue
          Where the "yui menu popup to the right side and to to the left, when in RTL mode"
          (It's on the assignment submissions page)

          Show
          Nadav Kavalerchik added a comment - Simon Can have a quick look at this issue and see if you can give a direction i can go into for solving this issue Where the "yui menu popup to the right side and to to the left, when in RTL mode" (It's on the assignment submissions page)
          Hide
          Nadav Kavalerchik added a comment -

          Hi Simon,
          I am not really assigning it to you, just asked for some guidance and help. (unless you have time to fix it, of course)

          Show
          Nadav Kavalerchik added a comment - Hi Simon, I am not really assigning it to you, just asked for some guidance and help. (unless you have time to fix it, of course)
          Hide
          Simon Coggins added a comment -

          Hi Nadav,

          The code displaying the menu is added in line 153 of mod/assign/module.js. It appears to be using a standard YUI plugin so the problem is that YUI isn't supporting RTL.

          The code that positions the menu appears to be lib/yuilib/3.6.0/build/node-menunav/node-menunav.js (that's the un-minified file, the compiled one that's actually called is called node-menunav-min.js).

          The code around line 902 (inside the _showMenu function) is where the offset is calculated:

          aXY[0] = aXY[0] + oLI.get(OFFSET_WIDTH);

          changing the + to a - does move the box (although not enough to correctly position it). You may need to calculate the size of the box so you can move it by that amount.

          Hope that helps!

          Simon

          Show
          Simon Coggins added a comment - Hi Nadav, The code displaying the menu is added in line 153 of mod/assign/module.js. It appears to be using a standard YUI plugin so the problem is that YUI isn't supporting RTL. The code that positions the menu appears to be lib/yuilib/3.6.0/build/node-menunav/node-menunav.js (that's the un-minified file, the compiled one that's actually called is called node-menunav-min.js). The code around line 902 (inside the _showMenu function) is where the offset is calculated: aXY [0] = aXY [0] + oLI.get(OFFSET_WIDTH); changing the + to a - does move the box (although not enough to correctly position it). You may need to calculate the size of the box so you can move it by that amount. Hope that helps! Simon
          Hide
          Nadav Kavalerchik added a comment -

          Simon

          Amazing analysis! Exactly what i was looking for. thanks
          I guess we (I) are not suppose to fix it and only report it upstream?

          Since I am not much familiar with YUI...
          Any chance we can override it, on the Moodle side, and change its behaviour?

          If not, I am suggesting a workaround...
          Is it acceptable to move the "Action" column near the "Grade" column,
          That way, the menu will popup in the middle of the table and not off of it.

          Show
          Nadav Kavalerchik added a comment - Simon Amazing analysis! Exactly what i was looking for. thanks I guess we (I) are not suppose to fix it and only report it upstream? Since I am not much familiar with YUI... Any chance we can override it, on the Moodle side, and change its behaviour? If not, I am suggesting a workaround... Is it acceptable to move the "Action" column near the "Grade" column, That way, the menu will popup in the middle of the table and not off of it.
          Hide
          Simon Coggins added a comment -

          We could probably do something hacky that would fix this particular instance but I agree a better approach would be to try to get better RTL support into YUI - otherwise it's just going to lead to merging issues when YUI gets updated.

          Do you know if there's a YUI expert who might know more about any existing or upcoming RTL support? I can imagine this would affect quite a few areas (like the main navigation menus).

          I think the workaround would be acceptable in the mean time.

          Simon

          Show
          Simon Coggins added a comment - We could probably do something hacky that would fix this particular instance but I agree a better approach would be to try to get better RTL support into YUI - otherwise it's just going to lead to merging issues when YUI gets updated. Do you know if there's a YUI expert who might know more about any existing or upcoming RTL support? I can imagine this would affect quite a few areas (like the main navigation menus). I think the workaround would be acceptable in the mean time. Simon
          Hide
          Nadav Kavalerchik added a comment -

          Link to relevant YUI3 bug report: http://yuilibrary.com/projects/yui3/ticket/2532695

          Show
          Nadav Kavalerchik added a comment - Link to relevant YUI3 bug report: http://yuilibrary.com/projects/yui3/ticket/2532695
          Hide
          Nadav Kavalerchik added a comment -

          I have added a link (branch: WIP-MDL-35249-master) to the suggested workaround. please review

          Show
          Nadav Kavalerchik added a comment - I have added a link (branch: WIP- MDL-35249 -master) to the suggested workaround. please review
          Hide
          Nadav Kavalerchik added a comment -

          Rebased over latest master (6-10-2012). Ready for review and integration

          Show
          Nadav Kavalerchik added a comment - Rebased over latest master (6-10-2012). Ready for review and integration
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Nadav Kavalerchik added a comment -

          Rebased over latest master (19-10-2012). Ready for review and integration

          Show
          Nadav Kavalerchik added a comment - Rebased over latest master (19-10-2012). Ready for review and integration
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Nadav,

          Can you please add some testing instructions for this.

          Andrew I've added you as a watcher here as you are the assignment lead.
          Would you please have a look at these changes and let us know your thoughts.

          This change circumvents the issue, and although normally I don't like such an approach (as it circumvents rather than solves) I actually think things look better with this patch! The column looks better placed next to the grade icon imho.
          As such it gets a +0.5 from me pending Andrew having a look at it as this is his area.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Nadav, Can you please add some testing instructions for this. Andrew I've added you as a watcher here as you are the assignment lead. Would you please have a look at these changes and let us know your thoughts. This change circumvents the issue, and although normally I don't like such an approach (as it circumvents rather than solves) I actually think things look better with this patch! The column looks better placed next to the grade icon imho. As such it gets a +0.5 from me pending Andrew having a look at it as this is his area. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Thanks Nadav, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Nadav, this has been integrated now
          Hide
          Frédéric Massart added a comment -

          Test passed. Although this does not fix the RTL problem, and I'm not entirely convinced about moving the edit column. Thanks anyway

          Show
          Frédéric Massart added a comment - Test passed. Although this does not fix the RTL problem, and I'm not entirely convinced about moving the edit column. Thanks anyway
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!
          Hide
          Aaron C Spike added a comment -

          I think this fix might be responsible for a regression. When clicking the link in the Grade column the wrong user page is displayed. This appears to be because the Edit column (userid column in the source) is responsible for initializing and incrementing the rownum member.

          if ($this->rownum < 0)

          { $this->rownum = $this->currpage * $this->pagesize; }

          else

          { $this->rownum += 1; }
          Show
          Aaron C Spike added a comment - I think this fix might be responsible for a regression. When clicking the link in the Grade column the wrong user page is displayed. This appears to be because the Edit column (userid column in the source) is responsible for initializing and incrementing the rownum member. if ($this->rownum < 0) { $this->rownum = $this->currpage * $this->pagesize; } else { $this->rownum += 1; }
          Hide
          Nadav Kavalerchik added a comment -

          Aaron, can you fix this? (Or should I)

          Show
          Nadav Kavalerchik added a comment - Aaron, can you fix this? (Or should I)
          Hide
          Nadav Kavalerchik added a comment -

          Aaron, I was about to fix this but could not reproduce the issue you reported.
          All links on the page seems to work just fine.
          I am using an updated Moodle 2.4dev master branch (10-11-2012)

          Show
          Nadav Kavalerchik added a comment - Aaron, I was about to fix this but could not reproduce the issue you reported. All links on the page seems to work just fine. I am using an updated Moodle 2.4dev master branch (10-11-2012)
          Hide
          Aaron C Spike added a comment -

          Nadav, We were experiencing the problem on Moodle 2.3.3 stable. I simply inverted this commit to fix it. A proper fix probably involves moving the code that increments rownum nearer to the beginning of the row (though this would still employ the use of side effects and future refactoring could trigger the problem once again). To be quite honest, I don't know where to find the 2.4dev branch so that I can read the code.

          Show
          Aaron C Spike added a comment - Nadav, We were experiencing the problem on Moodle 2.3.3 stable. I simply inverted this commit to fix it. A proper fix probably involves moving the code that increments rownum nearer to the beginning of the row (though this would still employ the use of side effects and future refactoring could trigger the problem once again). To be quite honest, I don't know where to find the 2.4dev branch so that I can read the code.
          Hide
          Dan Poltawski added a comment -

          Just FYI, this issue caused a big regression on the grading screen: MDL-36509

          Show
          Dan Poltawski added a comment - Just FYI, this issue caused a big regression on the grading screen: MDL-36509
          Hide
          Nadav Kavalerchik added a comment -

          Dan, I was sorry to read that. ( at the time, It looked like a simple change, "moving the table column from one place to the other" )

          I looked at MDL-36509, and I saw it is almost closed (at testing stage)
          Did you use Aaron's suggestion above to solve it?

          Show
          Nadav Kavalerchik added a comment - Dan, I was sorry to read that. ( at the time, It looked like a simple change, "moving the table column from one place to the other" ) I looked at MDL-36509 , and I saw it is almost closed (at testing stage) Did you use Aaron's suggestion above to solve it?
          Hide
          Aaron C Spike added a comment -

          Just read through the change set for MDL-36509. It does remove the side effect and decouple the column order from the increment. Looks good to me! Thanks everyone!

          Show
          Aaron C Spike added a comment - Just read through the change set for MDL-36509 . It does remove the side effect and decouple the column order from the increment. Looks good to me! Thanks everyone!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: