Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Add an RTL language package (Hebrew or Arabic or Farsi...) to Moodle (Home / ► Site administration / ► Language / ► Language packs)
      2. Switch to the RTL language you have installed by navigating to Moodle's Front-page and choosing it from the Language menu (or add "&lang=he" to the end of the URL you are currently using. If inside a course, set the RTL language from the course's setting)
        First sub issue (Login block)
      3. Set theme to Magazine and Logout
      4. View login block to see that "footer" is right aligned

        Second sub issue (Forum's discussions)
      5. Login and Navigate into a Course with a Forum activity that has some discussions (or add some yourself)
      6. Make sure the discussion's headers are right aligned

        Third sub issue (User's detailed view)
      7. Navigate into a Course that has one or more users enrolled into
      8. Click the Participants link under the course's navigation block
      9. Switch to Detailed view of user's properties
      10. Make sure the user's details are right aligned
      Show
      Add an RTL language package (Hebrew or Arabic or Farsi...) to Moodle (Home / ► Site administration / ► Language / ► Language packs) Switch to the RTL language you have installed by navigating to Moodle's Front-page and choosing it from the Language menu (or add "&lang=he" to the end of the URL you are currently using. If inside a course, set the RTL language from the course's setting) First sub issue (Login block) Set theme to Magazine and Logout View login block to see that "footer" is right aligned Second sub issue (Forum's discussions) Login and Navigate into a Course with a Forum activity that has some discussions (or add some yourself) Make sure the discussion's headers are right aligned Third sub issue (User's detailed view) Navigate into a Course that has one or more users enrolled into Click the Participants link under the course's navigation block Switch to Detailed view of user's properties Make sure the user's details are right aligned
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      WIP-MDL-35583

      Description

      RTL fixes to theme Canvas must be tested with some other ("based upon") theme (ex. Magazine, Arialist, Binarius) that uses Canvas as its parent theme.

      So, I am using Magazine for the following issues and tests

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            lazydaisy Mary Evans added a comment -

            @Sam I just realised I forgot to set this for Integration Review for Nadav...hope it's not too late!

            Show
            lazydaisy Mary Evans added a comment - @Sam I just realised I forgot to set this for Integration Review for Nadav...hope it's not too late!
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            nadavkav Nadav Kavalerchik added a comment -

            Rebased over latest master (29-9-2012)

            Show
            nadavkav Nadav Kavalerchik added a comment - Rebased over latest master (29-9-2012)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Rebased over latest master (6-10-2012). And ready for integration.

            Show
            nadavkav Nadav Kavalerchik added a comment - Rebased over latest master (6-10-2012). And ready for integration.
            Hide
            nebgor Aparup Banerjee added a comment -

            fyi: Nadav, i've just created MDL-35902 which may be of interest to you

            Show
            nebgor Aparup Banerjee added a comment - fyi: Nadav, i've just created MDL-35902 which may be of interest to you
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi, ok looking at this we need here:

            • test instructions
            • doesn't the padding-left need to be laterally inverted too? (not looked at page yet)

                
              +++ b/theme/canvas/style/mods.css
              @@ -33,6 +33,7 @@   
              padding-left:10px
               }
              +.dir-rtl .userinfobox .content {text-align: right;}

            Show
            nebgor Aparup Banerjee added a comment - Hi, ok looking at this we need here: test instructions doesn't the padding-left need to be laterally inverted too? (not looked at page yet) +++ b/theme/canvas/style/mods.css @@ -33,6 +33,7 @@ padding-left:10px } +.dir-rtl .userinfobox .content {text-align: right;}
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            I found the above issues while fixing issues in theme/magazine (MDL-35571) so all the Testing instructions are actually there. I think it is better to wait that MDL-35571 is integrated before I can pinpoint the right test instruction for this issue. ( I actually separated this issue out of MDL-35571 )

            Regarding the "doesn't the padding-left need to be laterally inverted too?"...
            You are right. I guess I missed that.

            Show
            nadavkav Nadav Kavalerchik added a comment - I found the above issues while fixing issues in theme/magazine ( MDL-35571 ) so all the Testing instructions are actually there. I think it is better to wait that MDL-35571 is integrated before I can pinpoint the right test instruction for this issue. ( I actually separated this issue out of MDL-35571 ) Regarding the "doesn't the padding-left need to be laterally inverted too?"... You are right. I guess I missed that.
            Hide
            nebgor Aparup Banerjee added a comment -

            holding this for MDL-35571 (and one tiny css fix and test instructions)

            Show
            nebgor Aparup Banerjee added a comment - holding this for MDL-35571 (and one tiny css fix and test instructions)
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi Nadav,

            247
            +.dir-rtl .userinfobox .content

            {text-align: right;}

            do you want to change that to invert the padding?

            also how about the test instructions as it seems MDL-35571 waiting for integration too now.

            Show
            nebgor Aparup Banerjee added a comment - Hi Nadav, 247 +.dir-rtl .userinfobox .content {text-align: right;} do you want to change that to invert the padding? also how about the test instructions as it seems MDL-35571 waiting for integration too now.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Hi Aparup,
            I have added your suggested fix and updated the branch.
            also, added some Test instruction with images. hope it is more clear
            Thanks!

            Show
            nadavkav Nadav Kavalerchik added a comment - Hi Aparup, I have added your suggested fix and updated the branch. also, added some Test instruction with images. hope it is more clear Thanks!
            Hide
            lazydaisy Mary Evans added a comment -

            @Nadav
            Like!

            Show
            lazydaisy Mary Evans added a comment - @Nadav Like!
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks Nadav, integrated into master now and up for testing.

            Show
            nebgor Aparup Banerjee added a comment - Thanks Nadav, integrated into master now and up for testing.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            I just noticed this is causing problem in serenity theme (didn't check other canvas themes).

            https://github.com/nadavkav/moodle/compare/master...WIP-MDL-35583#L1R664 is causing custom menu to be on multiple lines.
            Steps to reproduce:

            1. Add custom menu
            2. Check menu bar and it will be expanded.
            Show
            rajeshtaneja Rajesh Taneja added a comment - I just noticed this is causing problem in serenity theme (didn't check other canvas themes). https://github.com/nadavkav/moodle/compare/master...WIP-MDL-35583#L1R664 is causing custom menu to be on multiple lines. Steps to reproduce: Add custom menu Check menu bar and it will be expanded.
            Hide
            nebgor Aparup Banerjee added a comment -

            ah yea i've confirmed this

            Show
            nebgor Aparup Banerjee added a comment - ah yea i've confirmed this
            Hide
            rwijaya Rossiani Wijaya added a comment -

            I also confirm the issue. (attaching screenshots for magazine and serenity themes).

            Failing the test.

            Show
            rwijaya Rossiani Wijaya added a comment - I also confirm the issue. (attaching screenshots for magazine and serenity themes). Failing the test.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            @Rossiani
            I agree with you, thanks for stopping this, I just spotted this line in theme/canvas/core.css which should NOT be there.

            .yui3-menu .yui3-menu {position: static;}

            @Nadav
            The above CSS rule is NEW and is NOT relevant in every theme. If it relates to Magazine theme, then it should be in Magazine theme, IF and ONLY IF it's RTL related, NOT in Canvas theme!

            Show
            lazydaisy Mary Evans added a comment - - edited @Rossiani I agree with you, thanks for stopping this, I just spotted this line in theme/canvas/core.css which should NOT be there. .yui3-menu .yui3-menu {position: static;} @Nadav The above CSS rule is NEW and is NOT relevant in every theme. If it relates to Magazine theme, then it should be in Magazine theme, IF and ONLY IF it's RTL related, NOT in Canvas theme!
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            I did not add the following CSS rule

            .yui3-menu .yui3-menu {position: static;}

            And I do not see the problematic UI on my local version. weird.

            Show
            nadavkav Nadav Kavalerchik added a comment - I did not add the following CSS rule .yui3-menu .yui3-menu {position: static;} And I do not see the problematic UI on my local version. weird.
            Hide
            lazydaisy Mary Evans added a comment -

            You must have added it as it is in your branch compare...it's showing up as green so it's new.

            https://github.com/nadavkav/moodle/compare/master...WIP-MDL-35583

            Show
            lazydaisy Mary Evans added a comment - You must have added it as it is in your branch compare...it's showing up as green so it's new. https://github.com/nadavkav/moodle/compare/master...WIP-MDL-35583
            Hide
            nadavkav Nadav Kavalerchik added a comment - - edited

            Amazing. I have no idea what it is doing there. Me Bad. Sorry for the glitch. ( I even checked it before I wrote the above comment and did not see it. the mind can filter things that it does not want to see... Amazing! )

            Now, It is fixed

            Show
            nadavkav Nadav Kavalerchik added a comment - - edited Amazing. I have no idea what it is doing there. Me Bad. Sorry for the glitch. ( I even checked it before I wrote the above comment and did not see it. the mind can filter things that it does not want to see... Amazing! ) Now, It is fixed
            Hide
            lazydaisy Mary Evans added a comment -

            @Nadav,

            I've been trying to re-submit this for Integration Review, but don't seem to have the authority to do so as I cannot see the tabs to do this.

            It will have to wait now until someone reads this comment and comes and fix it.

            Sorry about thst.

            Show
            lazydaisy Mary Evans added a comment - @Nadav, I've been trying to re-submit this for Integration Review, but don't seem to have the authority to do so as I cannot see the tabs to do this. It will have to wait now until someone reads this comment and comes and fix it. Sorry about thst.
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks for fixing this up

            i've pulled that into integration master now as well as confirmed that the menu is back to normal for magazine and serenity here.

            ps: integration time fixes can be in separate commits if the patch hasn't been reverted yet. (easier to pull into integration imo)

            Show
            nebgor Aparup Banerjee added a comment - Thanks for fixing this up i've pulled that into integration master now as well as confirmed that the menu is back to normal for magazine and serenity here. ps: integration time fixes can be in separate commits if the patch hasn't been reverted yet. (easier to pull into integration imo)
            Hide
            lazydaisy Mary Evans added a comment -

            Excellent news...thank you!

            Show
            lazydaisy Mary Evans added a comment - Excellent news...thank you!
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Thank you Mary and Nadav for fixing this.

            It looks great now.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - Thank you Mary and Nadav for fixing this. It looks great now. Test passed.
            Hide
            nebgor Aparup Banerjee added a comment -

            Your issue has dug up some gold.
            It works great i've been told.
            Go forth, be brave, be bold.

            yay! "All your thoughts are belong to everyone."

            Thanks and ciao!

            Show
            nebgor Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12