Moodle
  1. Moodle
  2. MDL-35244

META: tasks related to RTL theme fixes before 2.4 freeze

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.4
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      1. Add one RTL language pack (Hebrew or Arabic or Farsi...) to Moodle
      2. Open each sub task and click the attached image
      ( That visually display the problematic UI element and the suggested solution )
      3. Make sure you are in LTR (English) and can see the same UI as the one attached to issue you are currently reviewing/testing.
      4. Apply the commit (patch/fix) from that specific issue. (Or the entire branch, from this Meta issue)
      5. Switch to Design Mode (Home / ► Site administration / ► Appearance / ► Themes / ► Theme settings)
      6. Switch to RTL UI by changing the language menu to "Hebrew" (or other languages, see list above)
      7. "Refresh" (CTRL-R / F5) the page you were viewing and make sure you see the problematic UI element... gone (Solved)
      8. go back to step 2 and choose another sub-task (issue) from the list.

      Only review or test the issues that are solved

      Show
      1. Add one RTL language pack (Hebrew or Arabic or Farsi...) to Moodle 2. Open each sub task and click the attached image ( That visually display the problematic UI element and the suggested solution ) 3. Make sure you are in LTR (English) and can see the same UI as the one attached to issue you are currently reviewing/testing. 4. Apply the commit (patch/fix) from that specific issue. (Or the entire branch, from this Meta issue) 5. Switch to Design Mode (Home / ► Site administration / ► Appearance / ► Themes / ► Theme settings) 6. Switch to RTL UI by changing the language menu to "Hebrew" (or other languages, see list above) 7. "Refresh" (CTRL-R / F5) the page you were viewing and make sure you see the problematic UI element... gone (Solved) 8. go back to step 2 and choose another sub-task (issue) from the list. Only review or test the issues that are solved
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      WIP-MDL-35244_RTL_fixes
    • Rank:
      43894

      Description

      This issue is a Meta issue to hold all the proposed fixes for the master branch before the 2.4 freeze
      (If you can come up with a better name... please do)

      I am adding several issues to this issue during the several future days (max: two weeks)
      starting today (4/9/2012) so please do not change this issue's status until i am done adding all the tasks (sub issues)

      Nadav

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for promoting this and providing fixes. I'll triage them now.

          Show
          Michael de Raadt added a comment - Thanks for promoting this and providing fixes. I'll triage them now.
          Hide
          Mary Evans added a comment -

          @Nadav

          When you come to do these, can I ask that you do each one separately and make sure that the Moodle version is the most up-to-date 'master' for the week you are submitting them. If you only manage to do one or two on this list that is OK. I just don't want to get into the position where they cause conficts when integrating. So as soon as they are done we get them in for Integration Review. Then wait for the next week and get more ready using up-to-date version of Moodle.

          If you are working on some of these now, then the ones you get finished by tomorrow evening need to be set for integration by Sunday at the latest. So I need to know before then so that I can do that.

          Thanks
          Mary

          Show
          Mary Evans added a comment - @Nadav When you come to do these, can I ask that you do each one separately and make sure that the Moodle version is the most up-to-date 'master' for the week you are submitting them. If you only manage to do one or two on this list that is OK. I just don't want to get into the position where they cause conficts when integrating. So as soon as they are done we get them in for Integration Review. Then wait for the next week and get more ready using up-to-date version of Moodle. If you are working on some of these now, then the ones you get finished by tomorrow evening need to be set for integration by Sunday at the latest. So I need to know before then so that I can do that. Thanks Mary
          Hide
          Nadav Kavalerchik added a comment -

          @Mary

          Thank you for reminding me!!! I keep updating my "master" branch before each fix.

          I have already proposed fixes for some of the issues. Here are the ones that should get a review and maybe even integration: MDL-35245 , MDL-35247 , MDL-35275.

          Show
          Nadav Kavalerchik added a comment - @Mary Thank you for reminding me!!! I keep updating my "master" branch before each fix. I have already proposed fixes for some of the issues. Here are the ones that should get a review and maybe even integration: MDL-35245 , MDL-35247 , MDL-35275 .
          Hide
          Nadav Kavalerchik added a comment - - edited

          @Mary

          MDL-35243 and MDL-35250 are also ready for peer (pear) review

          Show
          Nadav Kavalerchik added a comment - - edited @Mary MDL-35243 and MDL-35250 are also ready for peer ( pear ) review
          Hide
          Nadav Kavalerchik added a comment -

          @Mary

          And this one MDL-35426 too.

          Show
          Nadav Kavalerchik added a comment - @Mary And this one MDL-35426 too.
          Hide
          Mary Evans added a comment -

          Nadav
          Can I have test instructions for each one of these commits? Or one general instruction telling the tester where to look for each one?

          The fact these are all done in Base theme means that they should all get picked up in other themes which is good. So testing can be in any theme, but I would suggest Standard theme because if it does not work in Standard it will not work in other themes.

          To ADD Test Instruction go to Edit at the top of this page and you will find a section where you ADD the Test instructions.

          When this is done I can set it for Integration Review.

          Thanks

          Mary

          Show
          Mary Evans added a comment - Nadav Can I have test instructions for each one of these commits? Or one general instruction telling the tester where to look for each one? The fact these are all done in Base theme means that they should all get picked up in other themes which is good. So testing can be in any theme, but I would suggest Standard theme because if it does not work in Standard it will not work in other themes. To ADD Test Instruction go to Edit at the top of this page and you will find a section where you ADD the Test instructions. When this is done I can set it for Integration Review. Thanks Mary
          Hide
          Mary Evans added a comment -

          I've looked at the CSS in these commits they all look good, and I have no problem with them.

          Also you will most probably need to update your GITHUB and then rebase this branch before I set it for Intrgration Review.
          Thanks
          Mary

          Show
          Mary Evans added a comment - I've looked at the CSS in these commits they all look good, and I have no problem with them. Also you will most probably need to update your GITHUB and then rebase this branch before I set it for Intrgration Review. Thanks Mary
          Hide
          Nadav Kavalerchik added a comment -

          Github is updated with latest Moodle 2.4dev + MDLT-35244 branch is rebased on top of master.
          I have also added some Testing instructions (please make sure it make sense)

          Show
          Nadav Kavalerchik added a comment - Github is updated with latest Moodle 2.4dev + MDLT-35244 branch is rebased on top of master. I have also added some Testing instructions (please make sure it make sense)
          Hide
          Mary Evans added a comment -

          You should be able to keep working on this until Sunday. But not once Integration is underway. OK?
          If this is wrong way of doing things you will be told. So you will know for the next round.

          What I do is set each MDL issue for integration, on it's own, with test instructions for what to check when testing. See MDL-29561 as an example of my way of working.

          Show
          Mary Evans added a comment - You should be able to keep working on this until Sunday. But not once Integration is underway. OK? If this is wrong way of doing things you will be told. So you will know for the next round. What I do is set each MDL issue for integration, on it's own, with test instructions for what to check when testing. See MDL-29561 as an example of my way of working.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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 -

          WIP-MDL-35244_RTL_fixes branch was rebased just now (15-9-2012)

          Show
          Nadav Kavalerchik added a comment - WIP- MDL-35244 _RTL_fixes branch was rebased just now (15-9-2012)
          Hide
          Dan Poltawski added a comment -

          Hi Nadav,

          It was good to split this up into smaller issues compartmentalised.

          I discovered two issues with two of your patches:

          1) MDL-35275 - the fix for this seems like its not rtl specific:
          https://github.com/nadavkav/moodle/commit/e98458de46d546a9dad89358070afb649a52b371

          2) MDL-35250 - the fixes to tinymce are changing the tinymce upstream code. We import that from tinymce unmodified, and we shouldn't modify it in place. https://github.com/nadavkav/moodle/commit/48dbb9aa8b4165c37f1b19ae9d99ee81c83d8b4b

          In order to move this issue forward, I removed those patches so that they can be addressed later and integrated the other fixes to master.

          Show
          Dan Poltawski added a comment - Hi Nadav, It was good to split this up into smaller issues compartmentalised. I discovered two issues with two of your patches: 1) MDL-35275 - the fix for this seems like its not rtl specific: https://github.com/nadavkav/moodle/commit/e98458de46d546a9dad89358070afb649a52b371 2) MDL-35250 - the fixes to tinymce are changing the tinymce upstream code. We import that from tinymce unmodified, and we shouldn't modify it in place. https://github.com/nadavkav/moodle/commit/48dbb9aa8b4165c37f1b19ae9d99ee81c83d8b4b In order to move this issue forward, I removed those patches so that they can be addressed later and integrated the other fixes to master.
          Hide
          Dan Poltawski added a comment -

          Hi Nadav/Mary,

          With regards to how to deal with these issues, subtasks etc.

          I think that it is best if you create a separate branch for each issue, then we can integrate and test each issue in isolation and one issue doesn't depend on the other.

          This week, i've tried to 'fake that' by setting the subtasks which are integrated to 'waiting for testing', and removing the blocking patches myself from being integrated. But this isn't a great way of doing things because we have this one big issues which isn't completely resolved, and we'll have to handle the remaining issues individually.

          Show
          Dan Poltawski added a comment - Hi Nadav/Mary, With regards to how to deal with these issues, subtasks etc. I think that it is best if you create a separate branch for each issue, then we can integrate and test each issue in isolation and one issue doesn't depend on the other. This week, i've tried to 'fake that' by setting the subtasks which are integrated to 'waiting for testing', and removing the blocking patches myself from being integrated. But this isn't a great way of doing things because we have this one big issues which isn't completely resolved, and we'll have to handle the remaining issues individually.
          Hide
          Nadav Kavalerchik added a comment -

          Hi Dan,

          1. Although MDL-35275 does not seem like RTL specific. It is.
          When Moodle is running its INSTALL process there is no indication in the page class tag that we are in RTL mode.
          This is way the patch does not uses .dir-rtl classes. (Maybe a class is needed to be added to the code while UI is displayed in Hebrew,ie RTL mode)

          2. I know. that was exactly the point I was making in the MDL-35250 issue's comments. I was hoping you can suggest a workaround for this issue. without me trying to fix it upstream.

          I will separate each sub-task to a separate branch. and get back to you soon as it's done

          Show
          Nadav Kavalerchik added a comment - Hi Dan, 1. Although MDL-35275 does not seem like RTL specific. It is. When Moodle is running its INSTALL process there is no indication in the page class tag that we are in RTL mode. This is way the patch does not uses .dir-rtl classes. (Maybe a class is needed to be added to the code while UI is displayed in Hebrew,ie RTL mode) 2. I know. that was exactly the point I was making in the MDL-35250 issue's comments. I was hoping you can suggest a workaround for this issue. without me trying to fix it upstream. I will separate each sub-task to a separate branch. and get back to you soon as it's done
          Hide
          Dan Poltawski added a comment -

          Hi Nadav,

          No need to create seperate branches (apart from those two issues) as i've already integrated them. It'd be great if you could make sure the testing instructions on each subtask are done though.

          Show
          Dan Poltawski added a comment - Hi Nadav, No need to create seperate branches (apart from those two issues) as i've already integrated them. It'd be great if you could make sure the testing instructions on each subtask are done though.
          Hide
          Nadav Kavalerchik added a comment -

          @Dan

          I have added two new branches in my github account for those two issues (WIP-MDL-35250 and WIP-MDL-35275)
          I have updated the respective issues with those new branches and set testing instructions too.

          Please review those updates in the tracker and let me know if they are readable, so i can add more testing instruction to other sub tasks.

          Show
          Nadav Kavalerchik added a comment - @Dan I have added two new branches in my github account for those two issues (WIP- MDL-35250 and WIP- MDL-35275 ) I have updated the respective issues with those new branches and set testing instructions too. Please review those updates in the tracker and let me know if they are readable, so i can add more testing instruction to other sub tasks.
          Hide
          Dan Poltawski added a comment -

          Hi Nadav,

          Those testing instructions are great, thanks!

          Mary has suggested we move the non-fixed issues into a another issue, I think that would make sense so that we can close this issue cleanly.

          Show
          Dan Poltawski added a comment - Hi Nadav, Those testing instructions are great, thanks! Mary has suggested we move the non-fixed issues into a another issue, I think that would make sense so that we can close this issue cleanly.
          Hide
          Mary Evans added a comment -

          @Dan
          I have removed all the open issues and will list them tomorrow under a new META task.
          Thanks
          Mary

          Show
          Mary Evans added a comment - @Dan I have removed all the open issues and will list them tomorrow under a new META task. Thanks Mary
          Hide
          Adrian Greeve added a comment -

          All subtasks have been tested and passed. Passing this issue.

          Show
          Adrian Greeve added a comment - All subtasks have been tested and passed. Passing this issue.
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: