Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40752

Login panel should be placed on the right side, in RTL mode (theme/clean, bootstrap)

    Details

    • Testing Instructions:
      Hide
      • Enable registerauth
      1. Make sure you are not logged into Moodle
      2. Click the "login" link
      3. Switch to Hebrew (any RTL language)
      4. Make sure "Login" panel shows on the right side of the screen (as seen on the second screen capture)
      • repeat the test in clean, standard, and a few other themes.
      Show
      Enable registerauth Make sure you are not logged into Moodle Click the "login" link Switch to Hebrew (any RTL language) Make sure "Login" panel shows on the right side of the screen (as seen on the second screen capture) repeat the test in clean, standard, and a few other themes.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-40752-master
    • Sprint:
      FRONTEND Sprint 8
    • Story Points (Obsolete):
      5
    • Sprint:
      FRONTEND Sprint 8

      Description

      "login" and "register" panels should be flipped, in RTL mode

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for working on that, Nadav.

            Show
            salvetore Michael de Raadt added a comment - Thanks for working on that, Nadav.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Rebased onto latest 16/8/2013 master branch, and ready for peer-review.

            Show
            nadavkav Nadav Kavalerchik added a comment - Rebased onto latest 16/8/2013 master branch, and ready for peer-review.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Nadav,

            Looks good to me.

            This has been integrated to master.

            Cheers, Damyon

            Show
            damyon Damyon Wiese added a comment - Thanks Nadav, Looks good to me. This has been integrated to master. Cheers, Damyon
            Hide
            fred Frédéric Massart added a comment -

            Failing this as it has not been fixed in other themes than Clean.
            Question: Why isn't that backported?

            Show
            fred Frédéric Massart added a comment - Failing this as it has not been fixed in other themes than Clean. Question: Why isn't that backported?
            Hide
            fred Frédéric Massart added a comment -

            Sorry, I just realised that this issue was targetting Clean only. I still wonder why not fixing it other themes though .

            Anyway, please pass the test if my comments are irrelevant.

            Show
            fred Frédéric Massart added a comment - Sorry, I just realised that this issue was targetting Clean only. I still wonder why not fixing it other themes though . Anyway, please pass the test if my comments are irrelevant.
            Hide
            damyon Damyon Wiese added a comment -

            What other themes (I tested standard and it worked like this already).

            Good point about the backport - agree this is a bug and should be backported - will do now.

            Show
            damyon Damyon Wiese added a comment - What other themes (I tested standard and it worked like this already). Good point about the backport - agree this is a bug and should be backported - will do now.
            Hide
            damyon Damyon Wiese added a comment -

            Retested in standard and you are correct Fred - (I must have gone cross eyed the first time).

            Agree this is a fail without a matching patch for base. (Will hold on the backport to see if this gets reverted).

            Show
            damyon Damyon Wiese added a comment - Retested in standard and you are correct Fred - (I must have gone cross eyed the first time). Agree this is a fail without a matching patch for base. (Will hold on the backport to see if this gets reverted).
            Hide
            lazydaisy Mary Evans added a comment - - edited

            I think the reason for this is that historically Moodle did not work in older browsers when in RTL as a higher proportion of schools in the countries where RTL is mostly used were using IE7. This is why I changed the layout in Afterburner as this is the only Moodle theme which works in those browser as well as swap sideblocks in RTL.

            Show
            lazydaisy Mary Evans added a comment - - edited I think the reason for this is that historically Moodle did not work in older browsers when in RTL as a higher proportion of schools in the countries where RTL is mostly used were using IE7. This is why I changed the layout in Afterburner as this is the only Moodle theme which works in those browser as well as swap sideblocks in RTL.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            I think this can be cherry-picked into Moodle 2.5
            If not, let me know and I will backport it.

            Show
            nadavkav Nadav Kavalerchik added a comment - I think this can be cherry-picked into Moodle 2.5 If not, let me know and I will backport it.
            Hide
            damyon Damyon Wiese added a comment -

            The cherry pick is fine - but we also need this implemented in base. If it is not then I'll have to revert the patch and reopen the issue.

            Show
            damyon Damyon Wiese added a comment - The cherry pick is fine - but we also need this implemented in base. If it is not then I'll have to revert the patch and reopen the issue.
            Hide
            damyon Damyon Wiese added a comment -

            I have reverted the patch and am reopening this issue. Please add a patch for the base theme as well and resubmit for integration.

            Show
            damyon Damyon Wiese added a comment - I have reverted the patch and am reopening this issue. Please add a patch for the base theme as well and resubmit for integration.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            phalacee Jason Fowler added a comment -

            Taking over from Nadav to get this through.

            Show
            phalacee Jason Fowler added a comment - Taking over from Nadav to get this through.
            Hide
            phalacee Jason Fowler added a comment -

            I am having trouble getting the diff from Nadav's patch into my Stable 26 build, it seems the code is the same, which leads me to think there is no code in the patch here. But that can't be the case if Damyon was able to cherry pick it.

            Show
            phalacee Jason Fowler added a comment - I am having trouble getting the diff from Nadav's patch into my Stable 26 build, it seems the code is the same, which leads me to think there is no code in the patch here. But that can't be the case if Damyon was able to cherry pick it.
            Hide
            phalacee Jason Fowler added a comment -

            got it ...

            Show
            phalacee Jason Fowler added a comment - got it ...
            Hide
            phalacee Jason Fowler added a comment -

            Patches implemented, for base as well.

            Pushing for integration as this has been peer reviewed before.

            Show
            phalacee Jason Fowler added a comment - Patches implemented, for base as well. Pushing for integration as this has been peer reviewed before.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Jason,

            1. You haven't compiled the less changes in the commit, so I assume you haven't tested it. Please include the build files in your commit and ensure its been tested.

            2. Please leave a blank line on the second line of the git commit. It would've been preferable to keep Nadavs original commit and added your own on top I think to retain authorship

            3. Please get this peer reviewed.

            Show
            poltawski Dan Poltawski added a comment - Hi Jason, 1. You haven't compiled the less changes in the commit, so I assume you haven't tested it. Please include the build files in your commit and ensure its been tested. 2. Please leave a blank line on the second line of the git commit. It would've been preferable to keep Nadavs original commit and added your own on top I think to retain authorship 3. Please get this peer reviewed.
            Hide
            phalacee Jason Fowler added a comment -

            I must have forgotten to push the compiled less with the rest of the patch, as I do remember testing this. I tried keeping Nadav's original patch, but due to some mess with conflicts etc, I wrote it from scratch based on his branch.

            Show
            phalacee Jason Fowler added a comment - I must have forgotten to push the compiled less with the rest of the patch, as I do remember testing this. I tried keeping Nadav's original patch, but due to some mess with conflicts etc, I wrote it from scratch based on his branch.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Code change looks spot on thanks Jason.

            Just noting though that I agree with Dan - really Nadav's commit or at least authorship should be maintained here. That's something I feel very strongly about.
            If the conflict was an issue given his fix was dead easy I'd just copy it into the desired place like you have already and then set him as the author of the commit (--author='Nadav Kavalerchik <nadavkav@gmail.com>')

            Show
            samhemelryk Sam Hemelryk added a comment - Code change looks spot on thanks Jason. Just noting though that I agree with Dan - really Nadav's commit or at least authorship should be maintained here. That's something I feel very strongly about. If the conflict was an issue given his fix was dead easy I'd just copy it into the desired place like you have already and then set him as the author of the commit (--author='Nadav Kavalerchik <nadavkav@gmail.com>')
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Ooops - finishing the review.

            Show
            samhemelryk Sam Hemelryk added a comment - Ooops - finishing the review.
            Hide
            phalacee Jason Fowler added a comment -

            Changed the authorship of the commit, and swapped the message to say additional changes added by me. Pushing for integration now.

            Show
            phalacee Jason Fowler added a comment - Changed the authorship of the commit, and swapped the message to say additional changes added by me. Pushing for integration now.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Jason - integrated to master, 26 and 25

            Show
            poltawski Dan Poltawski added a comment - Thanks Jason - integrated to master, 26 and 25
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            Tested in 2.5, 2.6 and master using about half of all standard themes.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! Tested in 2.5, 2.6 and master using about half of all standard themes.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I won't be saying "Thanks!" this week. I'm tired of it.

            For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely.

            Just closing this as fixed, ciao

            PS: Just a bit of black/cruel humor, sorry, LOL!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I won't be saying "Thanks!" this week. I'm tired of it. For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely. Just closing this as fixed, ciao PS: Just a bit of black/cruel humor, sorry, LOL!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13