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

      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)

        Gliffy Diagrams

          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: