Details

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

      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

        Activity

        Hide
        Mary Evans added a comment -

        Hi Nadave, can you please add some Test Instructions?

        Show
        Mary Evans added a comment - Hi Nadave, can you please add some Test Instructions?
        Hide
        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
        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
        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
        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
        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
        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
        Nadav Kavalerchik added a comment -

        Great

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

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

        Show
        Nadav Kavalerchik added a comment - Eloy, what is smurf? what do i do with the xml file?
        Hide
        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
        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 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
        Nadav Kavalerchik added a comment -

        @Mary

        I will download the tool and fix this issue, soon

        Show
        Nadav Kavalerchik added a comment - @Mary I will download the tool and fix this issue, soon
        Hide
        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
        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
        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
        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
        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
        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
        Nadav Kavalerchik added a comment -

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

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

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

        Show
        Nadav Kavalerchik added a comment - Rebased over latest master (6-10-2012). Please code review and integrate.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Mary Evans added a comment -

        Taking this out of Integration Review to get it tidied up

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

        Thank you Mary

        Show
        Nadav Kavalerchik added a comment - Thank you Mary
        Hide
        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
        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
        Nadav Kavalerchik added a comment -

        I will. Thank you!

        Show
        Nadav Kavalerchik added a comment - I will. Thank you!
        Hide
        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
        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
        Mary Evans added a comment -

        Thanks DanP,

        I take it this does not need to be rebased?

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

        Rebased both branches

        Show
        Mary Evans added a comment - Rebased both branches
        Hide
        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
        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
        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
        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
        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
        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
        Aparup Banerjee added a comment -

        Agreed, this is still improvement overall

        integrated into 23 and master.

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

        passing, works for me. Thanks

        Show
        Aparup Banerjee added a comment - passing, works for me. Thanks
        Hide
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

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

        (not really)

        Closing, thanks!

        Show
        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: