Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.4
    • Fix Version/s: 2.3.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      TEST Requirements: RTL Language (eg:Hebrew (he)) pack to be installed. Enable Theme Designer Mode

      1. Select Magazine theme from Theme selector NOT by URL
      2. Go to home page (frontpage) and select RTL language from language menu.

      TEST to see the following list are all RIGHT aligned

      1. navbar/breadcrumbs
      2. site-name or course-name
      3. "edit" button
      4. logo
      5. WIKI page header titles

      TEST to see the following list are all LEFT aligned

      1. Login and user info

      TEST h3 heading (Frontpage specific)

      1. Frontpage > course list > teachers list
      2. Frontpage > "Categories and Courses" list > alignment
      3. Teachers' names list (under course names)
      Show
      TEST Requirements: RTL Language (eg:Hebrew (he)) pack to be installed. Enable Theme Designer Mode Select Magazine theme from Theme selector NOT by URL Go to home page (frontpage) and select RTL language from language menu. TEST to see the following list are all RIGHT aligned navbar/breadcrumbs site-name or course-name "edit" button logo WIKI page header titles TEST to see the following list are all LEFT aligned Login and user info TEST h3 heading (Frontpage specific) Frontpage > course list > teachers list Frontpage > "Categories and Courses" list > alignment Teachers' names list (under course names)
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-35571_master

      Description

      Frontpage and Course (and all other pages)

      1. Right align breadcrumbs
      2. right align site-name or course-name
      3. Left align "edit" button
      4. Left align Main-menu
      5. Switch left blocks column with right block column
      6. Right align logo
      7. Left align Course name
      8. Left align Login and user info

      Frontpage specific

      1. Frontpage > course list > add right side padding to the teachers list
      2. Frontpage > Ajax "Categories and Courses" list > fix alignments
      3. Add right padding to Teacher's names list (under course names)

      Other issues

      1. Right align discussion forum table header titles (derived from theme/canvas)
      2. Right align WIKI header titles

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            lazydaisy Mary Evans added a comment -

            Hi Nadave, can you please add some Test Instructions?

            Show
            lazydaisy Mary Evans added a comment - Hi Nadave, can you please add some Test Instructions?
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            I will. as soon as i finish to add some more fixes to this theme (I keep finding, while navigating around...)
            Please change status to "development in progress". It is not ready for "Waiting for integration review" yet.

            Show
            nadavkav Nadav Kavalerchik added a comment - I will. as soon as i finish to add some more fixes to this theme (I keep finding, while navigating around...) Please change status to "development in progress". It is not ready for "Waiting for integration review" yet.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            BTW, I found some issues that are related to theme/canvas which is one of the parents to this theme.
            Should i open a new MDL issue for those fixes and link it to this issue? or just commit those fixes as
            part of this issue (into this issue's branch)?

            Show
            nadavkav Nadav Kavalerchik added a comment - BTW, I found some issues that are related to theme/canvas which is one of the parents to this theme. Should i open a new MDL issue for those fixes and link it to this issue? or just commit those fixes as part of this issue (into this issue's branch)?
            Hide
            lazydaisy Mary Evans added a comment -

            I would open a new MDL and fix canvas separately. The integrators seem to prefer it that way. Besides it's easier to keep track.

            By the way I have asked Michael to see if you can have permissions so you can assign the issues you are fixing to yourself and when ready to set them for Integration Review. It would make it easy for me...at least!

            Show
            lazydaisy Mary Evans added a comment - I would open a new MDL and fix canvas separately. The integrators seem to prefer it that way. Besides it's easier to keep track. By the way I have asked Michael to see if you can have permissions so you can assign the issues you are fixing to yourself and when ready to set them for Integration Review. It would make it easy for me...at least!
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Great

            Show
            nadavkav Nadav Kavalerchik added a comment - Great
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi, I'm reopening this because there are some PHP coding style problems like:

            • AND OR (uppercase)
            • Missing space before/after " {" and "}

              "

            • elseif
            • ...

            I'm attaching one xml file (smurf.xml) showing all them. Please try to fix all the offending ones (note that some of the "X spaces" alignment ones could be false positives as far as the page is a mix of PHP and HTML.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi, I'm reopening this because there are some PHP coding style problems like: AND OR (uppercase) Missing space before/after " {" and "} " elseif ... I'm attaching one xml file (smurf.xml) showing all them. Please try to fix all the offending ones (note that some of the "X spaces" alignment ones could be false positives as far as the page is a mix of PHP and HTML.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Eloy, what is smurf? what do i do with the xml file?

            Show
            nadavkav Nadav Kavalerchik added a comment - Eloy, what is smurf? what do i do with the xml file?
            Hide
            lazydaisy Mary Evans added a comment -

            @Eloy
            Nadav is using the exact same code as is in theme/afterburner/layout/default.php
            https://github.com/nadavkav/moodle/blob/master/theme/afterburner/layout/default.php

            Sam integrated that a few weeks ago, if it is wrong now then it must have been wrong before. So I think Nadav was not too much aware of the (sometimes) strickt coding practice for developers.

            @Nadav
            There is a neat little code-checker you can download from the New Plugins Directory
            http://moodle.org/plugins/view.php?plugin=local_codechecker
            It's a collaborative work from Tim Hunt (Open University) && Eloy || STRONK7

            Show
            lazydaisy Mary Evans added a comment - @Eloy Nadav is using the exact same code as is in theme/afterburner/layout/default.php https://github.com/nadavkav/moodle/blob/master/theme/afterburner/layout/default.php Sam integrated that a few weeks ago, if it is wrong now then it must have been wrong before. So I think Nadav was not too much aware of the (sometimes) strickt coding practice for developers. @Nadav There is a neat little code-checker you can download from the New Plugins Directory http://moodle.org/plugins/view.php?plugin=local_codechecker It's a collaborative work from Tim Hunt (Open University) && Eloy || STRONK7
            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
            nadavkav Nadav Kavalerchik added a comment -

            @Mary

            I will download the tool and fix this issue, soon

            Show
            nadavkav Nadav Kavalerchik added a comment - @Mary I will download the tool and fix this issue, soon
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            @Nadav

            yes the attached smurf.xml file are the results of running both the codechecker and the moodlecheck utilities against your patch. But its main advantage is that it does not report ALL errors in the inspected code but only those introduced by your patch.

            That's the cause I attached the file, that is XML but pretty readable for humans too.

            @Mary

            if the same type of coding-style violations were accepted in another, similar, issue, it's that's issue integration fault (IMO), not this. Sometimes we don't run the checker, so it's possible the integrator there simply forgot that. When I was looking to changes, I detected some obvious ones and that leaded me to run the checker and report all them here.

            also note we are aiming, hopefully soon, to have those checkers being run automatically each time one issue is sent to integration. Results won't be .xml, but something more readable and ultimately will lead to auto-reopening (once we have the checkers tuned properly). So... the most developers knowing about the smurf.xml results, the best.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - @Nadav yes the attached smurf.xml file are the results of running both the codechecker and the moodlecheck utilities against your patch. But its main advantage is that it does not report ALL errors in the inspected code but only those introduced by your patch. That's the cause I attached the file, that is XML but pretty readable for humans too. @Mary if the same type of coding-style violations were accepted in another, similar, issue, it's that's issue integration fault (IMO), not this. Sometimes we don't run the checker, so it's possible the integrator there simply forgot that. When I was looking to changes, I detected some obvious ones and that leaded me to run the checker and report all them here. also note we are aiming, hopefully soon, to have those checkers being run automatically each time one issue is sent to integration. Results won't be .xml, but something more readable and ultimately will lead to auto-reopening (once we have the checkers tuned properly). So... the most developers knowing about the smurf.xml results, the best. Ciao
            Hide
            lazydaisy Mary Evans added a comment -

            In that case Eloy, ALL Moodle THEME layouts are wrong, according to the Code Checker, one way or another, as none carry the commented out information at the top of the page, which is the first thing the code checker checks for, after which it lists the errors and warnings. All the layouts are wrongly spaced and if statements and associated braces are in the wrong place, and not coded correctly either!

            Show
            lazydaisy Mary Evans added a comment - In that case Eloy, ALL Moodle THEME layouts are wrong, according to the Code Checker, one way or another, as none carry the commented out information at the top of the page, which is the first thing the code checker checks for, after which it lists the errors and warnings. All the layouts are wrongly spaced and if statements and associated braces are in the wrong place, and not coded correctly either!
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            @Eloy and @Mary
            Thank you for pointing out to me the codechecker plugin. It is very (very) usfull to know that code checkout correctly (when it does )

            I have installed and used the local/codechecker on the entire theme/magazine...
            I see a lot of code styling issues.

            I tryed to fix those I introduced with my patch. hope it is fixed now. (I reabse the patch on latest master and added a new commit to the branch)

            btw, I am not sure the codechecker is checking the code so good, since I got the following error/warrning:

            ········································if·(!right_to_left())·{
            142: Line indented incorrectly; expected 4 spaces, found 40

            Which seems to me a good coding style layout, when you consider it with the entire visual scope of the code.
            I do not wish to align that piece of code to 4 spaces. It is not readble.

            Show
            nadavkav Nadav Kavalerchik added a comment - @Eloy and @Mary Thank you for pointing out to me the codechecker plugin. It is very (very) usfull to know that code checkout correctly (when it does ) I have installed and used the local/codechecker on the entire theme/magazine... I see a lot of code styling issues. I tryed to fix those I introduced with my patch. hope it is fixed now. (I reabse the patch on latest master and added a new commit to the branch) btw, I am not sure the codechecker is checking the code so good, since I got the following error/warrning: ········································if·(!right_to_left())·{ 142: Line indented incorrectly; expected 4 spaces, found 40 Which seems to me a good coding style layout, when you consider it with the entire visual scope of the code. I do not wish to align that piece of code to 4 spaces. It is not readble.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            @Eloy, How do I run the codechecker only over my changes?

            Show
            nadavkav Nadav Kavalerchik added a comment - @Eloy, How do I run the codechecker only over my changes?
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Rebased over latest master (6-10-2012). Please code review and integrate.

            Show
            nadavkav Nadav Kavalerchik added a comment - Rebased over latest master (6-10-2012). Please code review and integrate.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            These changes will not be going in for integration until there is some feedback on what the situation is about getting these sideblock swapping changes into core, thus saving you/me/everyone a lot of trouble trying to fix every theme, which on the face of it, is crazy.

            Show
            lazydaisy Mary Evans added a comment - - edited These changes will not be going in for integration until there is some feedback on what the situation is about getting these sideblock swapping changes into core, thus saving you/me/everyone a lot of trouble trying to fix every theme, which on the face of it, is crazy.
            Hide
            lazydaisy Mary Evans added a comment -

            @Nadav
            We really need to sort this out as it is important for Moodle as well as RTL Community to discover what it is that's really needed. If it is just a matter of fixing which side the default blocks need to go when in RTL that can be done easily in core. If it is a matter of which blocks go where, then that is a matter for the Administrator. If blocks are not required in certain pages/courses then remove them. The blocks can be set to the side they need to be, the regions do not need to change.

            Show
            lazydaisy Mary Evans added a comment - @Nadav We really need to sort this out as it is important for Moodle as well as RTL Community to discover what it is that's really needed. If it is just a matter of fixing which side the default blocks need to go when in RTL that can be done easily in core. If it is a matter of which blocks go where, then that is a matter for the Administrator. If blocks are not required in certain pages/courses then remove them. The blocks can be set to the side they need to be, the regions do not need to change.
            Hide
            lazydaisy Mary Evans added a comment -

            @Sam

            I'll leave this up to you which commits get integrated. The fix for the "side-block swap" should not be in this but in a separate issue. But the other fixes are important.

            Show
            lazydaisy Mary Evans added a comment - @Sam I'll leave this up to you which commits get integrated. The fix for the "side-block swap" should not be in this but in a separate issue. But the other fixes are important.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            @Mary

            I agree, we need to sort it out. ( I thought we did, on a very basic level )

            Basically, when an RTL user uses Moodle, He/She needs to see its most important blocks on the top right corner of the screen (Settings and Navigation) this is how we (RTL folks) read and expect our attention to be focused, when using an online application(s). Switching to LTR, the blocks should switch too. Visually.

            Technically, we can set default block positions for the system or for a specific course format. So new courses are laid out correctly (when in RTL mode) when they are first created. but this will break LTR courses. which we use a lot. (Academic and K12 Israeli Moodle systems are a mix of 80% RTL courses with 20% of LTR courses).

            Using the "side-pre" and "side-post" in the code to switch side is not a perfect idea too, because it is just symbols of text and someone can create a new theme with different names for the regions which will break the themes response to mode change (drag and drop of blocks too). And I already saw some unique themes like that too.
            Considering the above difficulties, I think the current solution that we try to propagate to all the themes, is a relatively a good one.

            What do you think?

            Show
            nadavkav Nadav Kavalerchik added a comment - @Mary I agree, we need to sort it out. ( I thought we did, on a very basic level ) Basically, when an RTL user uses Moodle, He/She needs to see its most important blocks on the top right corner of the screen (Settings and Navigation) this is how we (RTL folks) read and expect our attention to be focused, when using an online application(s). Switching to LTR, the blocks should switch too. Visually. Technically, we can set default block positions for the system or for a specific course format. So new courses are laid out correctly (when in RTL mode) when they are first created. but this will break LTR courses. which we use a lot. (Academic and K12 Israeli Moodle systems are a mix of 80% RTL courses with 20% of LTR courses). Using the "side-pre" and "side-post" in the code to switch side is not a perfect idea too, because it is just symbols of text and someone can create a new theme with different names for the regions which will break the themes response to mode change (drag and drop of blocks too). And I already saw some unique themes like that too. Considering the above difficulties, I think the current solution that we try to propagate to all the themes, is a relatively a good one. What do you think?
            Hide
            lazydaisy Mary Evans added a comment -

            Taking this out of Integration Review to get it tidied up

            Show
            lazydaisy Mary Evans added a comment - Taking this out of Integration Review to get it tidied up
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Thank you Mary

            Show
            nadavkav Nadav Kavalerchik added a comment - Thank you Mary
            Hide
            lazydaisy Mary Evans added a comment - - edited

            OK, I think this will get a better reception and get fixed soon.

            I think in future when you do the RTL CSS fixes for a theme, that is all you commit to it. Nothing else. If you need to keep each fix (commit) in isolation, do it by creating a new MDL and state in the subject what the problem is then the commit message can just be 'MDL-xxxxx theme_themename: RTL CSS fix applied to style/core.css.

            This makes it easier when integrating and testing.

            Show
            lazydaisy Mary Evans added a comment - - edited OK, I think this will get a better reception and get fixed soon. I think in future when you do the RTL CSS fixes for a theme, that is all you commit to it. Nothing else. If you need to keep each fix (commit) in isolation, do it by creating a new MDL and state in the subject what the problem is then the commit message can just be 'MDL-xxxxx theme_themename: RTL CSS fix applied to style/core.css. This makes it easier when integrating and testing.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            I will. Thank you!

            Show
            nadavkav Nadav Kavalerchik added a comment - I will. Thank you!
            Hide
            poltawski Dan Poltawski 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
            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            poltawski Dan Poltawski 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 Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            lazydaisy Mary Evans added a comment -

            Thanks DanP,

            I take it this does not need to be rebased?

            Show
            lazydaisy Mary Evans added a comment - Thanks DanP, I take it this does not need to be rebased?
            Hide
            nebgor Aparup Banerjee 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
            nebgor Aparup Banerjee 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
            lazydaisy Mary Evans added a comment -

            I'm currenlty trying to resolve an issue while re-basing MDL-35571_M23 and hit a lot of problems due to the fact I wrote the git rebase commant incorrectly now I have MOODLE_23_STABLE diverging.

            I've since deleted the remote and local branches of MDL_35571_M23 with the hope of redoing it before Integration begins.

            In the event of this not happening, can whoever is integrating this cherry-pick the commit from MDL-35571_master as that is OK?

            Thanks

            Show
            lazydaisy Mary Evans added a comment - I'm currenlty trying to resolve an issue while re-basing MDL-35571 _M23 and hit a lot of problems due to the fact I wrote the git rebase commant incorrectly now I have MOODLE_23_STABLE diverging. I've since deleted the remote and local branches of MDL_35571_M23 with the hope of redoing it before Integration begins. In the event of this not happening, can whoever is integrating this cherry-pick the commit from MDL-35571 _master as that is OK? Thanks
            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
            lazydaisy Mary Evans added a comment -

            Rebased both branches

            Show
            lazydaisy Mary Evans added a comment - Rebased both branches
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi all,
            i've looked at this and here's what i could pick up:

            • breadcrumb arrows are pointing wrong way. (see span.arrow.sep ) could this be fixed up here too?
            • "edit" button (RIGHT aligned test (3) ) seems left. but it seems fine so maybe Test is wrong here. just need to verify.

            everything else seems to be fine.

            Show
            nebgor Aparup Banerjee added a comment - Hi all, i've looked at this and here's what i could pick up: breadcrumb arrows are pointing wrong way. (see span.arrow.sep ) could this be fixed up here too? "edit" button (RIGHT aligned test (3) ) seems left. but it seems fine so maybe Test is wrong here. just need to verify. everything else seems to be fine.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Aparup,

            1. Just now noticed the breadcrumb arrows funny. Thought they look fine to me... they definitely should be inverted. but it is not that important as for not passing this issue. (we'll fix it in another issue)
            2. The "EDIT" button should be left justified in RTL mode.
            Show
            nadavkav Nadav Kavalerchik added a comment - Aparup, Just now noticed the breadcrumb arrows funny. Thought they look fine to me... they definitely should be inverted. but it is not that important as for not passing this issue. (we'll fix it in another issue) The "EDIT" button should be left justified in RTL mode.
            Hide
            lazydaisy Mary Evans added a comment -

            Like Nadav says this is not too important with the arrows we can fix those i the next round of fixes.

            If you could pass this that would me great.
            Thanks

            Show
            lazydaisy Mary Evans added a comment - Like Nadav says this is not too important with the arrows we can fix those i the next round of fixes. If you could pass this that would me great. Thanks
            Hide
            nebgor Aparup Banerjee added a comment -

            Agreed, this is still improvement overall

            integrated into 23 and master.

            Show
            nebgor Aparup Banerjee added a comment - Agreed, this is still improvement overall integrated into 23 and master.
            Hide
            nebgor Aparup Banerjee added a comment -

            passing, works for me. Thanks

            Show
            nebgor Aparup Banerjee added a comment - passing, works for me. Thanks
            Hide
            nebgor Aparup Banerjee added a comment -

            @Nadav: i'll leave it you to change the arrows in another issue or not since they look fine to you. (perhaps my RTL brain was still in LTR mode, i've no idea :-D)

            Show
            nebgor Aparup Banerjee added a comment - @Nadav: i'll leave it you to change the arrows in another issue or not since they look fine to you. (perhaps my RTL brain was still in LTR mode, i've no idea :-D)
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            @Aparup, Thanks! your brain is just fine :- I will fix those arrows
            ( Though, philosophically speaking... you can see them as breadcrumbs for going back to the home-page and not arrows point out where you came from )

            Show
            nadavkav Nadav Kavalerchik added a comment - @Aparup, Thanks! your brain is just fine :- I will fix those arrows ( Though, philosophically speaking... you can see them as breadcrumbs for going back to the home-page and not arrows point out where you came from )
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

            (not really)

            Closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Nov/12