Moodle
  1. Moodle
  2. MDL-40752

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

    Details

    • Story Points (Obsolete):
      5
    • Sprint:
      FRONTEND Sprint 8

      Description

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

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for working on that, Nadav.

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

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

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

          Thanks Nadav,

          Looks good to me.

          This has been integrated to master.

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Thanks Nadav, Looks good to me. This has been integrated to master. Cheers, Damyon
          Hide
          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
          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
          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
          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 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 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 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 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
          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
          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
          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
          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 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 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 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 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 added a comment -

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

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

          Taking over from Nadav to get this through.

          Show
          Jason Fowler added a comment - Taking over from Nadav to get this through.
          Hide
          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
          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
          Jason Fowler added a comment -

          got it ...

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

          Patches implemented, for base as well.

          Pushing for integration as this has been peer reviewed before.

          Show
          Jason Fowler added a comment - Patches implemented, for base as well. Pushing for integration as this has been peer reviewed before.
          Hide
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Ooops - finishing the review.

          Show
          Sam Hemelryk added a comment - Ooops - finishing the review.
          Hide
          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
          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
          Dan Poltawski added a comment -

          Thanks Jason - integrated to master, 26 and 25

          Show
          Dan Poltawski added a comment - Thanks Jason - integrated to master, 26 and 25
          Hide
          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
          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
          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
          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:

                Agile