Details

      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)

        Gliffy Diagrams

          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: