Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      44297

      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

        Activity

        Hide
        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
        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
        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
        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
        Nadav Kavalerchik added a comment -

        Rebased over latest master (29-9-2012)

        Show
        Nadav Kavalerchik added a comment - Rebased over latest master (29-9-2012)
        Hide
        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
        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
        Nadav Kavalerchik added a comment -

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

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

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

        Show
        Aparup Banerjee added a comment - fyi: Nadav, i've just created MDL-35902 which may be of interest to you
        Hide
        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
        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
        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
        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
        Aparup Banerjee added a comment -

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

        Show
        Aparup Banerjee added a comment - holding this for MDL-35571 (and one tiny css fix and test instructions)
        Hide
        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
        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
        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
        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
        Mary Evans added a comment -

        @Nadav
        Like!

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

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

        Show
        Aparup Banerjee added a comment - Thanks Nadav, integrated into master now and up for testing.
        Hide
        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
        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
        Aparup Banerjee added a comment -

        ah yea i've confirmed this

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

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

        Failing the test.

        Show
        Rossiani Wijaya added a comment - I also confirm the issue. (attaching screenshots for magazine and serenity themes). Failing the test.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Mary Evans added a comment -

        Excellent news...thank you!

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

        Thank you Mary and Nadav for fixing this.

        It looks great now.

        Test passed.

        Show
        Rossiani Wijaya added a comment - Thank you Mary and Nadav for fixing this. It looks great now. Test passed.
        Hide
        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
        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: