Details

    • Rank:
      17180

      Description

      OK
      We are done, for now.
      Please review the following suggested changes
      regarding Moodle.org/Master branch (Aug-8-2011)

      This MDL is not ready for merge, yet.
      I am in the process of committing and pushing
      my local fixes to my github account (nadavkav/moodle).

      When i am done, i will mark this MDL as "ready for review"
      and then...

      Please see if you can PULL the following patches from my github account
      and add them to Moodle 2 master branch.

      Nadav,
      The "RTL & Hebrew Translation Team" (Shenkar College, Israel)

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for your continued work on this, Nadav.

          Show
          Michael de Raadt added a comment - Thanks for your continued work on this, Nadav.
          Hide
          Michael de Raadt added a comment -

          Hi, Nadav.

          Have you made any progress on this? We might shift this out of the current sprint until you have a complete solution.

          Michael;

          Show
          Michael de Raadt added a comment - Hi, Nadav. Have you made any progress on this? We might shift this out of the current sprint until you have a complete solution. Michael;
          Hide
          Nadav Kavalerchik added a comment -

          Hi Michael

          Sorry it took so much time to get back to you. I am, still, over-busy not just busy.
          I am in the process of clearing lots of patches and code updates from several projects and
          Moodle servers i was maintaing the last year. Moodle 1.9.x is still in a very high priority, here in Israel.

          So i have updated the links in this issue to reflact a more updated Moodle/master git tree.
          (Since i first branched it, a whilllleee ago)
          Hope the new and updated wip branch is easier to apply upstream. and hope all our suggestions
          are acceptable by you and the Moodle HQ team.

          We have "walked through" (QA) the entire Moodle (as of Aug-2011) and have no more major issues regarding RTL in the UI of the Moodle/Master system

          RTL wise, Moodle is ready (Not including the 95% translated strings, which will always be a work in progress, I suppose)

          One more wish, if at all possible...
          Have the blocks switch sides when switching to rtl mode. like Moodle 1.9 themes do.

          Thank you!

          Show
          Nadav Kavalerchik added a comment - Hi Michael Sorry it took so much time to get back to you. I am, still, over-busy not just busy. I am in the process of clearing lots of patches and code updates from several projects and Moodle servers i was maintaing the last year. Moodle 1.9.x is still in a very high priority, here in Israel. So i have updated the links in this issue to reflact a more updated Moodle/master git tree. (Since i first branched it, a whilllleee ago) Hope the new and updated wip branch is easier to apply upstream. and hope all our suggestions are acceptable by you and the Moodle HQ team. We have "walked through" (QA) the entire Moodle (as of Aug-2011) and have no more major issues regarding RTL in the UI of the Moodle/Master system RTL wise, Moodle is ready (Not including the 95% translated strings, which will always be a work in progress, I suppose) One more wish, if at all possible... Have the blocks switch sides when switching to rtl mode. like Moodle 1.9 themes do. Thank you!
          Hide
          Tim Hunt added a comment -

          Are you saying that this (i.e. https://github.com/nadavkav/moodle/compare/master...wip-MDL-27516-master-rtlpatches-727) is ready to go?

          If so, it would make things clearer if you could click the 'Request peer review...' button.

          I have just had a quick look at the changes, and they seem reasonable to me. In particular, +1 from me for the quiz parts of the changes.

          Show
          Tim Hunt added a comment - Are you saying that this (i.e. https://github.com/nadavkav/moodle/compare/master...wip-MDL-27516-master-rtlpatches-727 ) is ready to go? If so, it would make things clearer if you could click the 'Request peer review...' button. I have just had a quick look at the changes, and they seem reasonable to me. In particular, +1 from me for the quiz parts of the changes.
          Hide
          Tim Hunt added a comment -

          Oh, except that all your commit comments do not follow the Moodle house style. See http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

          You need to do a git rebase -i to fix all the commit comments.

          Show
          Tim Hunt added a comment - Oh, except that all your commit comments do not follow the Moodle house style. See http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages You need to do a git rebase -i to fix all the commit comments.
          Hide
          Nadav Kavalerchik added a comment -

          Hi Tim.
          I was not aware i should have clicked the button.
          Just did.

          BUT, accidently, set some unknown "Michael" user to review this issue, instead of Michael de Raadt.
          Please see if you can fix that.

          Sorry for the wrong style commits. i will fix it.

          Show
          Nadav Kavalerchik added a comment - Hi Tim. I was not aware i should have clicked the button. Just did. BUT, accidently, set some unknown "Michael" user to review this issue, instead of Michael de Raadt. Please see if you can fix that. Sorry for the wrong style commits. i will fix it.
          Hide
          Nadav Kavalerchik added a comment -

          Tim
          I need some help with "rebasing".
          I have tried :

          git rebase -i wip-MDL-27516-master-rtlpatches-727~27
          

          Then i get an editor with all the commits
          I add the MDL-27516 at the beginning of each commit's comment, and save the file.
          I get "Successfully rebased and updated refs/heads/wip-MDL-27516-master-rtlpatches-727"
          but the comments are not changed.

          Please advise

          Show
          Nadav Kavalerchik added a comment - Tim I need some help with "rebasing". I have tried : git rebase -i wip-MDL-27516-master-rtlpatches-727~27 Then i get an editor with all the commits I add the MDL-27516 at the beginning of each commit's comment, and save the file. I get "Successfully rebased and updated refs/heads/wip- MDL-27516 -master-rtlpatches-727" but the comments are not changed. Please advise
          Hide
          Nadav Kavalerchik added a comment -

          ok.
          found it.
          i had to use the term "reword" instead of "pick" in the rebase interactive editor.

          Show
          Nadav Kavalerchik added a comment - ok. found it. i had to use the term "reword" instead of "pick" in the rebase interactive editor.
          Hide
          Nadav Kavalerchik added a comment -

          Pushing it was not easy too
          But i think, was finally done.
          Please check my github/nadavkav/moodle wip-MDL-27516-master-rtlpatches-727
          to make sure i did not break any refs

          Here is the end of the drama :

          git push -v github wip-MDL-27516-master-rtlpatches-727
          
          Pushing to git@github.com:nadavkav/moodle.git
          To git@github.com:nadavkav/moodle.git
           ! [rejected]        wip-MDL-27516-master-rtlpatches-727 -> wip-MDL-27516-master-rtlpatches-727 (non-fast-forward)
          error: failed to push some refs to 'git@github.com:nadavkav/moodle.git'
          To prevent you from losing history, non-fast-forward updates were rejected
          Merge the remote changes (e.g. 'git pull') before pushing again.  See the
          'Note about fast-forwards' section of 'git push --help' for details.
          
          root@devbook:/var/www/moodle-org/master/moodle# git push -f github wip-MDL-27516-master-rtlpatches-727
          
          
          Show
          Nadav Kavalerchik added a comment - Pushing it was not easy too But i think, was finally done. Please check my github/nadavkav/moodle wip- MDL-27516 -master-rtlpatches-727 to make sure i did not break any refs Here is the end of the drama : git push -v github wip-MDL-27516-master-rtlpatches-727 Pushing to git@github.com:nadavkav/moodle.git To git@github.com:nadavkav/moodle.git ! [rejected] wip-MDL-27516-master-rtlpatches-727 -> wip-MDL-27516-master-rtlpatches-727 (non-fast-forward) error: failed to push some refs to 'git@github.com:nadavkav/moodle.git' To prevent you from losing history, non-fast-forward updates were rejected Merge the remote changes (e.g. 'git pull') before pushing again. See the 'Note about fast-forwards' section of 'git push --help' for details. root@devbook:/ var /www/moodle-org/master/moodle# git push -f github wip-MDL-27516-master-rtlpatches-727
          Hide
          Tim Hunt added a comment -

          That looks better. Hopefully someone from Moodle HQ will take a look soon.

          Show
          Tim Hunt added a comment - That looks better. Hopefully someone from Moodle HQ will take a look soon.
          Hide
          Sam Hemelryk added a comment -

          Hi Nadav,

          Excellent work on the patch!

          I think before it is ready for integration there is a couple of things that need to be tidied up, however all in all your changes look spot on!

          1. Remove the newline added to the end of mod/quiz/styles.php
          2. Remove font-size:100% from mod/workshop/styles.css, really this should be done for both rtl and ltr languages although in that case it would still best be purposed in a separate bug so that David can check it out.
          3. theme/base/style/core.css For the direction ltr changes I think they should be grouped together (remove the newlines between them and use commas so that there is a big group together and just one {direction: ltr}

            )

          If you have time to make those changes that would be most appreciated, otherwise just let me know and I'll make them for you Once done this can certainly go up for integration.

          One more thing, I see there are 27 commits, do you mind if for integration we squash it down to one commit?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Nadav, Excellent work on the patch! I think before it is ready for integration there is a couple of things that need to be tidied up, however all in all your changes look spot on! Remove the newline added to the end of mod/quiz/styles.php Remove font-size:100% from mod/workshop/styles.css, really this should be done for both rtl and ltr languages although in that case it would still best be purposed in a separate bug so that David can check it out. theme/base/style/core.css For the direction ltr changes I think they should be grouped together (remove the newlines between them and use commas so that there is a big group together and just one {direction: ltr} ) If you have time to make those changes that would be most appreciated, otherwise just let me know and I'll make them for you Once done this can certainly go up for integration. One more thing, I see there are 27 commits, do you mind if for integration we squash it down to one commit? Cheers Sam
          Hide
          Nadav Kavalerchik added a comment - - edited

          Hi Sam
          Thank you for your kind words.

          Please feel free to change those tiny changes (i have moved on and my git tree has already changed) and squash them too
          Or leave them out and i will fixed them in my new WIP-MDL...

          Show
          Nadav Kavalerchik added a comment - - edited Hi Sam Thank you for your kind words. Please feel free to change those tiny changes (i have moved on and my git tree has already changed) and squash them too Or leave them out and i will fixed them in my new WIP-MDL...
          Hide
          Nadav Kavalerchik added a comment -

          Eloy
          Please see if you can integrate these changes before Moodle 2.2 is locked.

          Show
          Nadav Kavalerchik added a comment - Eloy Please see if you can integrate these changes before Moodle 2.2 is locked.
          Hide
          Mary Evans added a comment - - edited

          Well Nadav,

          I am not sure if I am allowed to ask for this to be integrated, were these changes not the same as the ones Sam added recently?

          With a bit of luck Eloy might see this and tell me what to do next...because I have a horrid feeling this is NOT the correct procedure.

          Cheers
          Mary

          Show
          Mary Evans added a comment - - edited Well Nadav, I am not sure if I am allowed to ask for this to be integrated, were these changes not the same as the ones Sam added recently? With a bit of luck Eloy might see this and tell me what to do next...because I have a horrid feeling this is NOT the correct procedure. Cheers Mary
          Hide
          Nadav Kavalerchik added a comment -

          If these changes are already in, that's great

          I'll run a "git pull --purge" as soon as there is a confirmation on that to make sure it shows on my local version
          before i continue to send more RTL fixes. (I have a few new ones, but i guess they are for 2.2.1)

          Thanks

          Show
          Nadav Kavalerchik added a comment - If these changes are already in, that's great I'll run a "git pull --purge" as soon as there is a confirmation on that to make sure it shows on my local version before i continue to send more RTL fixes. (I have a few new ones, but i guess they are for 2.2.1) Thanks
          Hide
          Sam Hemelryk added a comment -

          Hi guys.
          Thanks Nadav sorry this got skipped over for so long.
          I've tidied up the patch and condensed it down to a single commit.
          It's now been integrated and has been backported to 2.1 as well.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys. Thanks Nadav sorry this got skipped over for so long. I've tidied up the patch and condensed it down to a single commit. It's now been integrated and has been backported to 2.1 as well. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Tested during integration

          Show
          Sam Hemelryk added a comment - Tested during integration
          Hide
          Mary Evans added a comment -

          Yipee!!!

          Show
          Mary Evans added a comment - Yipee!!!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Thanks! Closing...

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Thanks! Closing...

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: