Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      Testing Instructions

      For all checks, you will need to be in RTL mode. getting into RTL mode is possible after installing an RTL language pack. for example Hebrew. Now, you can set RTL mode system wide or just inside the course you are checking. (use course's setting menu for that)

      1. As Administrator, enable Activity Completion feature and as a Student, open up a course with several activities and make sure the Activity Completion boxes are displayed on the right side of the activities, adjacent to the activities' names.

      2. As a Teacher go through the setting of several (one or more, since it is all the same) Activities to make sure the fitemtitle (right form's div) and the felement (left form's div) are justified to the right and are proportionally aligned to the right. in each Form.

      3. As a Teacher, open course grade overview report and check that the grade category title(s) (on the top of the table) are justified to the right.

      4. As a Teacher, click the grades link inside the course' setting menu. now, when in grade overview report, click the Scales link on the settings' menu. Now, make sure the Scales table is centred on the screen. (btw, i think it should be the same for LTR mode)

      5. As a Teacher, click into a Glossary activity, now, click Categories. now, click Edit Categories. now, check that the categories table is centred. (btw, i think it should be the same for LTR mode) and all table's cells are right aligned. (add some categories, if necessary )

      6. As any user, click on the user's name to view its profile page. check to "fullprofile" link is right justified

      7. As a Teacher, click into an Assignment. now, click into assignment's submissions. check that the submissions table is right aligned.

      Show
      Testing Instructions For all checks, you will need to be in RTL mode. getting into RTL mode is possible after installing an RTL language pack. for example Hebrew. Now, you can set RTL mode system wide or just inside the course you are checking. (use course's setting menu for that) 1. As Administrator, enable Activity Completion feature and as a Student, open up a course with several activities and make sure the Activity Completion boxes are displayed on the right side of the activities, adjacent to the activities' names. 2. As a Teacher go through the setting of several (one or more, since it is all the same) Activities to make sure the fitemtitle (right form's div) and the felement (left form's div) are justified to the right and are proportionally aligned to the right. in each Form. 3. As a Teacher, open course grade overview report and check that the grade category title(s) (on the top of the table) are justified to the right. 4. As a Teacher, click the grades link inside the course' setting menu. now, when in grade overview report, click the Scales link on the settings' menu. Now, make sure the Scales table is centred on the screen. (btw, i think it should be the same for LTR mode) 5. As a Teacher, click into a Glossary activity, now, click Categories. now, click Edit Categories. now, check that the categories table is centred. (btw, i think it should be the same for LTR mode) and all table's cells are right aligned. (add some categories, if necessary ) 6. As any user, click on the user's name to view its profile page. check to "fullprofile" link is right justified 7. As a Teacher, click into an Assignment. now, click into assignment's submissions. check that the submissions table is right aligned.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30361-master-rtlpatches
    • Rank:
      32969

      Description

      More, RTL patches. mostly to CSS files.
      With some exceptions, for Modules that uses hard coded alignment styles
      (which need to be fixed, probably)

      URL, currently points to a temporary branch on my github account.
      It will be renamed to reflect this MDL issue. (soon)

        Activity

        Hide
        Mary Evans added a comment - - edited

        Sam & Eloy I have just added you as watchers with this.

        This is going to become a regular thing while Nadav is adding CSS fixes for RTL

        I just hope what I am trying to do with this project is the correct approach...so any helpful comments would be very much appreciated.

        Thanks
        Mary

        Show
        Mary Evans added a comment - - edited Sam & Eloy I have just added you as watchers with this. This is going to become a regular thing while Nadav is adding CSS fixes for RTL I just hope what I am trying to do with this project is the correct approach...so any helpful comments would be very much appreciated. Thanks Mary
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        This patch needs a lot more work sorry:

        1. mod/glossary.php.
          • There is a couple of typo's there $rightalignemnt
          • There are a couple of places in code where variables have been used in HTML but have not been wrapped in php tags. If you create a category and then delete it you will see what I mean by inspecting the HTML on the confirm screen. (look for $rightalignment).
        2. theme/base/style/course.css
          • Please check all of the CSS that has been added for #page-course-report-* and #page-course-user. In master most of those reports have been moved and course/user.php has been greatly refactored.
        3. I have a bit of a concern with some of the CSS that has come through with these changes. RTL changes should be attempting to fix display issues only, the design of the theme's should be kept as close to LTR as possible (helps reduce the number of rtl rules we need).
          THe changes in theme/base/style/user.css and theme/standard/style/grade.css appear to be design rather than functionality.
        4. Please please please put more information into the issue (testing instructions) and backport these changes as they are bug fixes.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, This patch needs a lot more work sorry: mod/glossary.php. There is a couple of typo's there $rightalignemnt There are a couple of places in code where variables have been used in HTML but have not been wrapped in php tags. If you create a category and then delete it you will see what I mean by inspecting the HTML on the confirm screen. (look for $rightalignment). theme/base/style/course.css Please check all of the CSS that has been added for #page-course-report-* and #page-course-user. In master most of those reports have been moved and course/user.php has been greatly refactored. I have a bit of a concern with some of the CSS that has come through with these changes. RTL changes should be attempting to fix display issues only, the design of the theme's should be kept as close to LTR as possible (helps reduce the number of rtl rules we need). THe changes in theme/base/style/user.css and theme/standard/style/grade.css appear to be design rather than functionality. Please please please put more information into the issue (testing instructions) and backport these changes as they are bug fixes. Cheers Sam
        Hide
        Nadav Kavalerchik added a comment -

        Hi Sam

        1. fixed. and pushed a new commit : https://github.com/nadavkav/moodle/commit/6b0c67e89a22598a2a66ebdb26a8b6fe3a9309e7

        2. I have ran through all the course and user's reports. they all look good. i do not see need to update #page-course-report-* and #page-course-user.

        3. These are important and subtle. (not fonts, colors or other weight attributes are used) anyways, you call.

        4. I try to commit each UI change with as much info to where the change was. what Moodle component and what page. I will try some more. hope it helps.

        Thank you
        Nadav

        Show
        Nadav Kavalerchik added a comment - Hi Sam 1. fixed. and pushed a new commit : https://github.com/nadavkav/moodle/commit/6b0c67e89a22598a2a66ebdb26a8b6fe3a9309e7 2. I have ran through all the course and user's reports. they all look good. i do not see need to update #page-course-report-* and #page-course-user. 3. These are important and subtle. (not fonts, colors or other weight attributes are used) anyways, you call. 4. I try to commit each UI change with as much info to where the change was. what Moodle component and what page. I will try some more. hope it helps. Thank you Nadav
        Hide
        Mary Evans added a comment -

        Hi Nadav,
        It's not the description of the commits that Sam is talking about, he is requesting you ADD some "TEST INSTRUCTION" in this page! This is so that the person who has to 'Test what has changed' to see if the changes made to the code, work.

        There should be a section at the top of this page, which is not shown because you have not added anything. I'll edit this page and activate the Test Instructions box so you can see it, and then I would like you to fill in the missing information.

        Basically all you need to do is add a numbered list, something like:
        1. change language menu to RTL language
        2. go to page xyz and test whatever...etc., etc.,

        Cheers
        Mary

        Show
        Mary Evans added a comment - Hi Nadav, It's not the description of the commits that Sam is talking about, he is requesting you ADD some "TEST INSTRUCTION" in this page! This is so that the person who has to 'Test what has changed' to see if the changes made to the code, work. There should be a section at the top of this page, which is not shown because you have not added anything. I'll edit this page and activate the Test Instructions box so you can see it, and then I would like you to fill in the missing information. Basically all you need to do is add a numbered list, something like: 1. change language menu to RTL language 2. go to page xyz and test whatever...etc., etc., Cheers Mary
        Hide
        Mary Evans added a comment -

        When you have added the instructions and you have fixed whatever needed wrapping in PHP which Sam mentioned then I can set it to Waiting for integration review" as Sam took it out of the review stage. As it is at the moment this issue is just open and nothing will get done.

        Thanks
        Mary

        Show
        Mary Evans added a comment - When you have added the instructions and you have fixed whatever needed wrapping in PHP which Sam mentioned then I can set it to Waiting for integration review" as Sam took it out of the review stage. As it is at the moment this issue is just open and nothing will get done. Thanks Mary
        Hide
        Mary Evans added a comment -

        Hi Nadav,

        I have added you to MDL-30361, because I am not convinced you are receiving the updates from this page, otherwise I think you would have answered my previous comment!

        These patches wont get added to Moodle without Test Instructions, and also the patches will need re-basing now that Moodle/master has been updated, so whenever you are ready can we get this show on the road!

        Thanks

        Show
        Mary Evans added a comment - Hi Nadav, I have added you to MDL-30361 , because I am not convinced you are receiving the updates from this page, otherwise I think you would have answered my previous comment! These patches wont get added to Moodle without Test Instructions, and also the patches will need re-basing now that Moodle/master has been updated, so whenever you are ready can we get this show on the road! Thanks
        Hide
        Mary Evans added a comment -

        Further to the patches you submitted in this particular tracker issue, how about we split them up into BUGS and IMPROVEMENTS? This way the BUG commits will be easier to manage and more likely to get integrated, and the improvements can go into a theme I am making.

        Cheers
        Mary

        Show
        Mary Evans added a comment - Further to the patches you submitted in this particular tracker issue, how about we split them up into BUGS and IMPROVEMENTS? This way the BUG commits will be easier to manage and more likely to get integrated, and the improvements can go into a theme I am making. Cheers Mary
        Hide
        Mary Evans added a comment -

        Thanks Nadav...we seem to be getting somewhere now

        Show
        Mary Evans added a comment - Thanks Nadav...we seem to be getting somewhere now
        Hide
        Sam Hemelryk added a comment -

        Hi guys,
        This has been integrated now.
        I did revert the CSS for course report. Nadav I think your checkout may be a little behind. All of the reports the CSS was targeting have been moved and no longer have the #page-course-report - they are now #page-report, and likely wise for the path classes.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, This has been integrated now. I did revert the CSS for course report. Nadav I think your checkout may be a little behind. All of the reports the CSS was targeting have been moved and no longer have the #page-course-report - they are now #page-report, and likely wise for the path classes. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Tested during integration

        Show
        Sam Hemelryk added a comment - Tested during integration
        Hide
        Nadav Kavalerchik added a comment -

        Sam,
        Did you fix my old CSS #page-course-report fixes?
        Should i update my local git tree (now) and fix them and send a new commit for review?

        Show
        Nadav Kavalerchik added a comment - Sam, Did you fix my old CSS #page-course-report fixes? Should i update my local git tree (now) and fix them and send a new commit for review?
        Hide
        Mary Evans added a comment - - edited

        Don't do anything now to this as it's all gone through.

        If you need to add more then start a new sub-task in MDL-30337
        But wait until we get confirmation from Eloy to say that this issue is FIXED and CLOSED
        this makes it easier all round to keep tabs on where we are with all these fixes.

        Thanks for all you do with these RTL fixes Nadav, it is very much appreciated.

        Mary

        Show
        Mary Evans added a comment - - edited Don't do anything now to this as it's all gone through. If you need to add more then start a new sub-task in MDL-30337 But wait until we get confirmation from Eloy to say that this issue is FIXED and CLOSED this makes it easier all round to keep tabs on where we are with all these fixes. Thanks for all you do with these RTL fixes Nadav, it is very much appreciated. Mary
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days.

        Thanks for the hard work! Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days. Thanks for the hard work! Closing, ciao
        Hide
        Sam Hemelryk added a comment -

        Hi Nadav,

        I do appologise, it's not often I modify to such an extent during integration, normally things get sent back so that they can be properly discussed and fixed up.
        In this case I decided to split out the CSS I wasn't comfortable with in order to get the rest in before the impending release in the next couple of days.

        In regards to the course report CSS no I didn't fix them up, something funny had gone on with the #page-course-report css that came through with the patch as several of the styles already existed.
        One more thing to note about CSS for those reports is that because the reports are now proper plugins they can have their own css in report/reportname/styles.css
        I would recommend adding styles there in the future rather than in the theme's unless it is theme specific.

        The following CSS is the CSS that was not integrated and needs to be reviewed:

        #page-course-user .section {margin-left: 30px;margin-right: 30px;margin-bottom: 20px;border-width:1px;border-style:solid;padding:10px;}
        #page-course-user .section .content {margin-left: 30px;margin-right: 30px;}
        #page-course-user .section h2 {margin-top: 0;}
        #page-course-user .info {margin:10px;}
        
        #page-course-report-log-index .info,
        #page-course-report-log-indexlive .info {margin:10px;}
        #page-course-report-stats-index .graph {margin-bottom: 1em;}
        
        #page-course-report .logselectform,
        #page-course-report .participationselectform,
        #page-course-report-log-index .logselectform,
        #page-course-report-participation-index .participationselectform {margin:10px auto;}
        #page-course-report .participationselectform label,
        #page-course-report-participation-index .participationselectform label {margin-left:15px;margin-right:5px;}
        
        .path-course-report-outline td.numviews {text-align:right;}
        .path-course-report-outline tr.section {text-align: center;}
        

        So as mentioned the course reports have moved to report/reportname, and the course user script (#page-course-user) has greatly changed as it no longer supports course reports and instead redirects to them.

        As Mary mentioned feel free to open a new issue after reviewing the CSS to put any of it back up for integration, or sneak it in with other RTL theme fixes.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Nadav, I do appologise, it's not often I modify to such an extent during integration, normally things get sent back so that they can be properly discussed and fixed up. In this case I decided to split out the CSS I wasn't comfortable with in order to get the rest in before the impending release in the next couple of days. In regards to the course report CSS no I didn't fix them up, something funny had gone on with the #page-course-report css that came through with the patch as several of the styles already existed. One more thing to note about CSS for those reports is that because the reports are now proper plugins they can have their own css in report/reportname/styles.css I would recommend adding styles there in the future rather than in the theme's unless it is theme specific. The following CSS is the CSS that was not integrated and needs to be reviewed: #page-course-user .section {margin-left: 30px;margin-right: 30px;margin-bottom: 20px;border-width:1px;border-style:solid;padding:10px;} #page-course-user .section .content {margin-left: 30px;margin-right: 30px;} #page-course-user .section h2 {margin-top: 0;} #page-course-user .info {margin:10px;} #page-course-report-log-index .info, #page-course-report-log-indexlive .info {margin:10px;} #page-course-report-stats-index .graph {margin-bottom: 1em;} #page-course-report .logselectform, #page-course-report .participationselectform, #page-course-report-log-index .logselectform, #page-course-report-participation-index .participationselectform {margin:10px auto;} #page-course-report .participationselectform label, #page-course-report-participation-index .participationselectform label {margin-left:15px;margin-right:5px;} .path-course-report-outline td.numviews {text-align:right;} .path-course-report-outline tr.section {text-align: center;} So as mentioned the course reports have moved to report/reportname, and the course user script (#page-course-user) has greatly changed as it no longer supports course reports and instead redirects to them. As Mary mentioned feel free to open a new issue after reviewing the CSS to put any of it back up for integration, or sneak it in with other RTL theme fixes. Cheers Sam
        Hide
        Nadav Kavalerchik added a comment -

        Hi Sam

        This make perfect sense to me and I agree with the move.
        Thought i do not see any reports folders under my course/report folder
        (just updated my local tree with github/moodle.org/master )

        I guess we will continue this, after 2.2 is out. and every thing is more stable to work on

        Thank you

        Show
        Nadav Kavalerchik added a comment - Hi Sam This make perfect sense to me and I agree with the move. Thought i do not see any reports folders under my course/report folder (just updated my local tree with github/moodle.org/master ) I guess we will continue this, after 2.2 is out. and every thing is more stable to work on Thank you

          People

          • Votes:
            2 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: