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

          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