Uploaded image for project: '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

      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)

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              nadavkav 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
              nadavkav 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
              nadavkav 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
              nadavkav 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
              simoncoggins 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
              simoncoggins 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
              nadavkav 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
              nadavkav 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
              simoncoggins 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
              simoncoggins 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
              nadavkav Nadav Kavalerchik added a comment -

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

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

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

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

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

              Show
              nadavkav Nadav Kavalerchik added a comment - Rebased over latest master (6-10-2012). Ready for review and integration
              Hide
              nebgor 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
              nebgor 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
              nadavkav Nadav Kavalerchik added a comment -

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

              Show
              nadavkav Nadav Kavalerchik added a comment - Rebased over latest master (19-10-2012). Ready for review and integration
              Hide
              poltawski 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
              poltawski 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
              samhemelryk 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
              samhemelryk 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
              samhemelryk Sam Hemelryk added a comment -

              Thanks Nadav, this has been integrated now

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Nadav, this has been integrated now
              Hide
              fred 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
              fred 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

              (not really)

              Closing, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!
              Hide
              acspike 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
              acspike 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
              nadavkav Nadav Kavalerchik added a comment -

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

              Show
              nadavkav Nadav Kavalerchik added a comment - Aaron, can you fix this? (Or should I)
              Hide
              nadavkav 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
              nadavkav 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
              acspike 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
              acspike 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Just FYI, this issue caused a big regression on the grading screen: MDL-36509
              Hide
              nadavkav 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
              nadavkav 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
              acspike 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
              acspike 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:
                    Fix Release Date:
                    12/Nov/12