Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Select Splash theme in the normal way via Theme selector.
      2. Test every area, login, frontpage, course pages, topic, weekly, reports, admin reports, quiz, activities, resources etc., for consistency of design.
      3. Test blocks both docked and un-docked for pagelayout, again for consistency of design.
      4. Test Splash Custom theme settings in Appearance > Themes > Splash
      5. Test to see that the embedded page works correctly by adding a resource page that contains an embedded file such as a PDF.
      Show
      Select Splash theme in the normal way via Theme selector. Test every area, login, frontpage, course pages, topic, weekly, reports, admin reports, quiz, activities, resources etc., for consistency of design. Test blocks both docked and un-docked for pagelayout, again for consistency of design. Test Splash Custom theme settings in Appearance > Themes > Splash Test to see that the embedded page works correctly by adding a resource page that contains an embedded file such as a PDF.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31351_master
    • Rank:
      37868

      Description

      This is a total update of theme_splash intended for Moodle 2.3, as the changes applied may affect customised versions of this theme that are in use on production sites.

        Activity

        Hide
        Helen Foster added a comment -

        Mary, hope you don't mind me adding you as a watcher in the hope you can help with this issue.

        Show
        Helen Foster added a comment - Mary, hope you don't mind me adding you as a watcher in the hope you can help with this issue.
        Hide
        Mary Evans added a comment -

        Hi Helen,
        Glad to help.
        I have been working on this theme in my spare time and was about to make some drastic changes.
        However, it look like Caroline, had the same thoughts.

        I'm happy to Peer Review this also and present it for Integration Review eventually.
        .
        Cheers

        Show
        Mary Evans added a comment - Hi Helen, Glad to help. I have been working on this theme in my spare time and was about to make some drastic changes. However, it look like Caroline, had the same thoughts. I'm happy to Peer Review this also and present it for Integration Review eventually. . Cheers
        Hide
        Helen Foster added a comment -

        Mary, you are a star! Also Caroline - thanks a lot for your patch.

        Show
        Helen Foster added a comment - Mary, you are a star! Also Caroline - thanks a lot for your patch.
        Hide
        Mary Evans added a comment -

        @Sam,
        I've just added you as a watcher here. I'm a bit concerned that the changes which Caroline as added. If you have a moment can take a look at them?
        Thanks

        Show
        Mary Evans added a comment - @Sam, I've just added you as a watcher here. I'm a bit concerned that the changes which Caroline as added. If you have a moment can take a look at them? Thanks
        Hide
        Mary Evans added a comment -

        @Caroline,

        Hi, thanks for taking the time to update this theme.

        I've just changed the PULL branch URLs to show the correct view...I used to get this wrong at first.

        I'll be Peer Reviewing this, if I have any problems I'll get help from Sam or Davo.

        If all is OK I'll submit it for Integration Review.

        Cheers
        Mary

        Show
        Mary Evans added a comment - @Caroline, Hi, thanks for taking the time to update this theme. I've just changed the PULL branch URLs to show the correct view...I used to get this wrong at first. I'll be Peer Reviewing this, if I have any problems I'll get help from Sam or Davo. If all is OK I'll submit it for Integration Review. Cheers Mary
        Hide
        Mary Evans added a comment -

        @Caroline

        Can you tell me...

        1. Why is pagebg.jpg in the Splash root directory?
        2. Are all the images added in this commit necessary, if so how do they differ from the originals?

        Thanks
        Mary

        Show
        Mary Evans added a comment - @Caroline Can you tell me... Why is pagebg.jpg in the Splash root directory? Are all the images added in this commit necessary, if so how do they differ from the originals? Thanks Mary
        Hide
        Caroline Kennedy added a comment -

        Hi Mary

        Sorry - No idea how the pagebg.jpg got there, should be safe to remove.

        As for the images, I haven't added new images to the theme, only deleted some redundant ones. Might have been a permissions change?

        Kind Regards,
        Caroline

        Show
        Caroline Kennedy added a comment - Hi Mary Sorry - No idea how the pagebg.jpg got there, should be safe to remove. As for the images, I haven't added new images to the theme, only deleted some redundant ones. Might have been a permissions change? Kind Regards, Caroline
        Hide
        Mary Evans added a comment -

        In that case, can you remove the images, unless they are new, as the images which are already in master work fine.

        Basically you only need to add the files you altered, so can you re-work this and submit master branch only. I can cherry-pick the changes to MOODLE_22_STABLE and MOODLE_21_STABLE if necessary.

        Thanks

        Mary

        Show
        Mary Evans added a comment - In that case, can you remove the images, unless they are new, as the images which are already in master work fine. Basically you only need to add the files you altered, so can you re-work this and submit master branch only. I can cherry-pick the changes to MOODLE_22_STABLE and MOODLE_21_STABLE if necessary. Thanks Mary
        Hide
        Mary Evans added a comment - - edited

        @Caroline

        Would you rather I took care of this from here?

        I could get it ready for this weeks PULL then, all being well, will be in the next upgrade version of Moodle.

        Cheers
        Mary

        Show
        Mary Evans added a comment - - edited @Caroline Would you rather I took care of this from here? I could get it ready for this weeks PULL then, all being well, will be in the next upgrade version of Moodle. Cheers Mary
        Hide
        Caroline Kennedy added a comment -

        Hi Mary

        Been very busy today, so I am sorry for the late reply.

        I created the theme from scratch using the files and images from the current Splash theme. Alot of the updates were regarding layout issues in the css, which I have resolved.

        If you would not mind taking care of it from here, to prepare it for the next upgrade, that would be great.

        Thank you very much.

        Caroline

        Show
        Caroline Kennedy added a comment - Hi Mary Been very busy today, so I am sorry for the late reply. I created the theme from scratch using the files and images from the current Splash theme. Alot of the updates were regarding layout issues in the css, which I have resolved. If you would not mind taking care of it from here, to prepare it for the next upgrade, that would be great. Thank you very much. Caroline
        Hide
        Mary Evans added a comment -

        Great news!...I'll get on with it now!.
        Cheers
        Mary

        Show
        Mary Evans added a comment - Great news!...I'll get on with it now!. Cheers Mary
        Hide
        Mary Evans added a comment -

        @Caroline

        In getting SPLASH ready for integration, I have redone the changes you submitted here into a new branch in my github.

        To save time at a later date, I am in the process of cleaning up the CSS, Validating it and fixing typos at the same time, there was a text-decoration: italic; instead of font-style: italic; easily done...I've put border-weight instead of border-width, before now, in some themes ages ago...and I am still finding them! LOL

        OK...what I did try out...as I found quite a lot of duplication of the stylesheets for the colours, was to put all the page layout for the theme in a stylesheet called pagelayout.css, then take out all the layout from css file for the colours, which I think makes for easier debugging in the long run. Change sl.css to red.css again easier for the Site Administrator to find the color code if needed. Putting all this together the theme test really well and the layout is consistent as I did find it wasn't always so. So basically all the colour css files contain the identical elements but different colours. I also removed the custom css settings from the end of core.css to a new settings.css which is placed as the end of the list, this ensures that the custom settings are the last to be read, as the css it read from top to bottom of the list or left to right in the array, which is opposite to the way parent themes are read which is 'right to left'. Crazy I know but true.

        What I am asking is, are you willing to let me submit these changes to test them out? I could get someone to Peer Review them if you approve.

        Cheers
        Mary

        Show
        Mary Evans added a comment - @Caroline In getting SPLASH ready for integration, I have redone the changes you submitted here into a new branch in my github. To save time at a later date, I am in the process of cleaning up the CSS, Validating it and fixing typos at the same time, there was a text-decoration: italic; instead of font-style: italic; easily done...I've put border-weight instead of border-width, before now, in some themes ages ago...and I am still finding them! LOL OK...what I did try out...as I found quite a lot of duplication of the stylesheets for the colours, was to put all the page layout for the theme in a stylesheet called pagelayout.css, then take out all the layout from css file for the colours, which I think makes for easier debugging in the long run. Change sl.css to red.css again easier for the Site Administrator to find the color code if needed. Putting all this together the theme test really well and the layout is consistent as I did find it wasn't always so. So basically all the colour css files contain the identical elements but different colours. I also removed the custom css settings from the end of core.css to a new settings.css which is placed as the end of the list, this ensures that the custom settings are the last to be read, as the css it read from top to bottom of the list or left to right in the array, which is opposite to the way parent themes are read which is 'right to left'. Crazy I know but true. What I am asking is, are you willing to let me submit these changes to test them out? I could get someone to Peer Review them if you approve. Cheers Mary
        Hide
        Mary Evans added a comment -

        I think this is as good as it gets...and looking good.

        Show
        Mary Evans added a comment - I think this is as good as it gets...and looking good.
        Hide
        Caroline Kennedy added a comment -

        Hi Mary - Brilliant.. thanks for all of your help. Yes I am fine for that to be submitted.

        Caroline

        Show
        Caroline Kennedy added a comment - Hi Mary - Brilliant.. thanks for all of your help. Yes I am fine for that to be submitted. Caroline
        Hide
        Mary Evans added a comment - - edited

        @Caroline

        Thanks it's now in for integration...but I have just realised in all the activity of last night I forgot to change the PULL information!

        I've just changes the repository links...hopefully not too late in the day!

        Cheers
        Mary

        Show
        Mary Evans added a comment - - edited @Caroline Thanks it's now in for integration...but I have just realised in all the activity of last night I forgot to change the PULL information! I've just changes the repository links...hopefully not too late in the day! Cheers Mary
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Sorry, I'm reopening this:

        • Along the issue you comment about to pick the changes for 22 and 21. But finally it's only for master. Can you confirm which are the target branches for this.
        • Some testing instructions, specially about what has changed, apart from the cleanup would be welcome.
        • As far as this includes new files and general cleanup, it should 100% fulfill Moodle's coding style and documenting style rules. The changes are plagued of missing first copyright lines, bad indentation, missing file phpdocs, whitespace...

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this: Along the issue you comment about to pick the changes for 22 and 21. But finally it's only for master. Can you confirm which are the target branches for this. Some testing instructions, specially about what has changed, apart from the cleanup would be welcome. As far as this includes new files and general cleanup, it should 100% fulfill Moodle's coding style and documenting style rules. The changes are plagued of missing first copyright lines, bad indentation, missing file phpdocs, whitespace... Ciao
        Hide
        Mary Evans added a comment - - edited

        @John Stabinger

        I hope you can take this on as I have spent too long with it already and looking at the comment from Eloy...was a shear waste of my time too.

        I don't think Caroline has much time to spare either.

        If you can't do this then I'll come back to it after I have calmed down

        Thanks

        Over and out...

        Show
        Mary Evans added a comment - - edited @John Stabinger I hope you can take this on as I have spent too long with it already and looking at the comment from Eloy...was a shear waste of my time too. I don't think Caroline has much time to spare either. If you can't do this then I'll come back to it after I have calmed down Thanks Over and out...
        Hide
        Mary Evans added a comment -

        @Eloy

        Is there a Moodle course I could go on to learn all this stuff about white space and such?

        Cheers

        Show
        Mary Evans added a comment - @Eloy Is there a Moodle course I could go on to learn all this stuff about white space and such? Cheers
        Hide
        Helen Foster added a comment -

        Mary, you are a STAR! Please don't feel bad about Eloy's comment. He has a challenging job being an integrator, always having to explain his decisions and it can be difficult not to sound abrupt. I'm sure Eloy didn't mean for you to feel bad.

        I guess info on white space and such can be found in http://docs.moodle.org/dev/Coding_style and maybe http://docs.moodle.org/dev/Coding though you'll notice that the guidelines are still under construction

        Hope you're feeling better soon, and please don't forget - you are a STAR!

        Show
        Helen Foster added a comment - Mary, you are a STAR! Please don't feel bad about Eloy's comment. He has a challenging job being an integrator, always having to explain his decisions and it can be difficult not to sound abrupt. I'm sure Eloy didn't mean for you to feel bad. I guess info on white space and such can be found in http://docs.moodle.org/dev/Coding_style and maybe http://docs.moodle.org/dev/Coding though you'll notice that the guidelines are still under construction Hope you're feeling better soon, and please don't forget - you are a STAR!
        Hide
        John Stabinger added a comment -

        Helen is right on, you do great work. I can help cleaning up, if that is what is needed.

        Show
        John Stabinger added a comment - Helen is right on, you do great work. I can help cleaning up, if that is what is needed.
        Hide
        Mary Evans added a comment -

        @Helen,
        Thanks for the moral booster!
        I think I have allowed my batteries to burn out...I need to recharge.
        Most of my problem with this is that there really is a lack of information, or consistency.

        I'm following Git for Developers which I have a printout that I amend for my own personal way of working, so it is full of notes. And more often than not work well...until the other day when I made a mistake when rebasing...now my whole GIT master branch is screwed up.

        So with that problem coupled with this issue...which I was hoping would have at least worked, but I now see, thanks to Eloy, who was right to tell me where the problems are, that I did this too quick and did not think ahead enough, and so made more mistakes.

        Anyway...I shall fix this as I think this is a lovely theme, and worth that extra mile.

        Thanks Helen and you are a STAR too!

        Show
        Mary Evans added a comment - @Helen, Thanks for the moral booster! I think I have allowed my batteries to burn out...I need to recharge. Most of my problem with this is that there really is a lack of information, or consistency. I'm following Git for Developers which I have a printout that I amend for my own personal way of working, so it is full of notes. And more often than not work well...until the other day when I made a mistake when rebasing...now my whole GIT master branch is screwed up. So with that problem coupled with this issue...which I was hoping would have at least worked, but I now see, thanks to Eloy, who was right to tell me where the problems are, that I did this too quick and did not think ahead enough, and so made more mistakes. Anyway...I shall fix this as I think this is a lovely theme, and worth that extra mile. Thanks Helen and you are a STAR too!
        Hide
        Mary Evans added a comment -

        @John

        I am so sorry to have bothered you, as I know you are under pressure too.
        If I need any help I'll let you know, but really it's only a matter of fixing some CSS pages...at least I think that's all it is! I'll put them through the CSS Tidy and spin then round a bit...give them a good shake and hang them out to dry in this lovely cold fresh Siberian blast we are getting in the UK.

        Show
        Mary Evans added a comment - @John I am so sorry to have bothered you, as I know you are under pressure too. If I need any help I'll let you know, but really it's only a matter of fixing some CSS pages...at least I think that's all it is! I'll put them through the CSS Tidy and spin then round a bit...give them a good shake and hang them out to dry in this lovely cold fresh Siberian blast we are getting in the UK.
        Hide
        Mary Evans added a comment -

        I'm back!

        Show
        Mary Evans added a comment - I'm back!
        Hide
        Mary Evans added a comment -

        @Eloy

        I have taking on-board all you have said, concerning the problems with this commit, and I will try and fix all those you mentioned, however, I am not clear about what you mean by "missing file phpdocs"? Can you elaborate?

        Cheers
        Mary

        Show
        Mary Evans added a comment - @Eloy I have taking on-board all you have said, concerning the problems with this commit, and I will try and fix all those you mentioned, however, I am not clear about what you mean by "missing file phpdocs"? Can you elaborate? Cheers Mary
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        Hi, super-Mary!

        Please don't worry! My comments weren't "grrring" at all. Just aiming to improve the patch a bit before allowing it to land.

        While I'm not looking for perfection at all in the incoming patches, I think that there are some basic rules that should be fulfilled, specially if the issue is, itself, a cleanup, aka, the goal is keep the plugin (theme) better.

        So I only asked for:

        1) The target versions where this was supposed to land. As said above, it confused me a bit that, some comments before sending to integration, you comment about rebasing and cherry-picking, and finally the submitted patch is only for master. Hence, I was asking for clarification.

        2) There are some very-basic rules, like:

        • 4 space indenting
        • add copyright/phpdocs to all new files
        • no "incorrect" whitespace (like lines indented with tabs instead of 4-whitespace chars, or trailing spaces in lines).

        3) Some testing instructions. If the theme includes one new layout, say "embed", I think it's correct to request you to define some testing instructions like: "Create one new xxx module, with this settings and check that it displays correctly with "splash". Or whatever, just something that can be checked to verify that the theme is working as expected.

        So, once again, don't worry at all, plz! I think all the points above are really easy to fulfill. If you need support, no problem at all, just ask. Sincerely!

        Also, note that you can verify all the 1) and 2) points above simply by installing the codechecker local plugin and running it against the theme/splash directory, it will reveal all the problems commented above.

        You can get it from:

        Or also directly from git, so by pulling it from time to time, you'll get it automatically updated with more rules, from:

        https://github.com/moodlehq/moodle-local_codechecker

        Just rename the dir to "codechecker", move it to your moodle/local directory, log as admin (so it will be installed) and look in admin->developer->code checker. Enter "theme/splash" and you will get a complete report of things to fix, specially those marked as errors.

        It's just that, nothing more and nothing else. In a near future we have plans to perform those checks, and some others (like the phpdocs checker) automatically, so you will get the complete report here, in the issue, everytime that you send something to integration. But for now, the only way to verify it is to execute the code-checker as explained above.

        And that's all. Third time, don't worry nor feel bad for my words. They were 100% blaming-free, but constructive. Your continue effort with themes is much-much appreciated here and, I'm sure that, with a bit more of organization/comunication, you will be able to fulfill all those rules easily.

        Hugs and thanks, Eloy

        PS: The smile means "happy" in Spanish. Don't worry unless you see some (puke) (moaning) (punch), that will mean I'm "grrr-ing", LOL, joking!

        Show
        Eloy Lafuente (stronk7) added a comment - - edited Hi, super-Mary! Please don't worry! My comments weren't "grrring" at all. Just aiming to improve the patch a bit before allowing it to land. While I'm not looking for perfection at all in the incoming patches, I think that there are some basic rules that should be fulfilled, specially if the issue is, itself, a cleanup, aka, the goal is keep the plugin (theme) better. So I only asked for: 1) The target versions where this was supposed to land. As said above, it confused me a bit that, some comments before sending to integration, you comment about rebasing and cherry-picking, and finally the submitted patch is only for master. Hence, I was asking for clarification. 2) There are some very-basic rules, like: 4 space indenting add copyright/phpdocs to all new files no "incorrect" whitespace (like lines indented with tabs instead of 4-whitespace chars, or trailing spaces in lines). 3) Some testing instructions. If the theme includes one new layout, say "embed", I think it's correct to request you to define some testing instructions like: "Create one new xxx module, with this settings and check that it displays correctly with "splash". Or whatever, just something that can be checked to verify that the theme is working as expected. So, once again, don't worry at all, plz! I think all the points above are really easy to fulfill. If you need support, no problem at all, just ask. Sincerely! Also, note that you can verify all the 1) and 2) points above simply by installing the codechecker local plugin and running it against the theme/splash directory, it will reveal all the problems commented above. You can get it from: http://moodle.org/plugins/view.php?plugin=local_codechecker Or also directly from git, so by pulling it from time to time, you'll get it automatically updated with more rules, from: https://github.com/moodlehq/moodle-local_codechecker Just rename the dir to "codechecker", move it to your moodle/local directory, log as admin (so it will be installed) and look in admin->developer->code checker. Enter "theme/splash" and you will get a complete report of things to fix, specially those marked as errors. It's just that, nothing more and nothing else. In a near future we have plans to perform those checks, and some others (like the phpdocs checker) automatically, so you will get the complete report here, in the issue, everytime that you send something to integration. But for now, the only way to verify it is to execute the code-checker as explained above. And that's all. Third time, don't worry nor feel bad for my words. They were 100% blaming-free, but constructive. Your continue effort with themes is much-much appreciated here and, I'm sure that, with a bit more of organization/comunication, you will be able to fulfill all those rules easily. Hugs and thanks, Eloy PS: The smile means "happy" in Spanish. Don't worry unless you see some (puke) (moaning) (punch), that will mean I'm "grrr-ing", LOL, joking!
        Hide
        Mary Evans added a comment - - edited

        @Eloy Please can you cast a critical eye over my latest revisions for this issue when - you have a few moments to spare that is?

        Show
        Mary Evans added a comment - - edited @Eloy Please can you cast a critical eye over my latest revisions for this issue when - you have a few moments to spare that is?
        Hide
        Mary Evans added a comment - - edited

        @Eloy

        Forget the Peer Review, as I feel I am in a "NO WIN" situation here.
        I have been using the Code Checker which works great...only thing is I don't understand some of the developer jargon. Anyway, I was trying to get to grips with it, and thought I would code check theme/base/layout/general.php as I was sure the code in THAT file would be Ok...but how wrong could I was!

        Here's the printout.

        Files found: 1
        
            theme\base\layout\general.php - 28 error(s) and 3 warning(s)
        
        Total: 28 error(s) and 3 warning(s)
        theme\base\layout\general.php
        
            2: Line 1 of the opening comment must start "// This file is part of".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 2 of the opening comment must start "//".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 3 of the opening comment must start "// Moodle is free software: you can redistribute it and/or modify".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 4 of the opening comment must start "// it under the terms of the GNU General Public License as published by".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 5 of the opening comment must start "// the Free Software Foundation, either version 3 of the License, or".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 6 of the opening comment must start "// (at your option) any later version.".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 7 of the opening comment must start "//".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 8 of the opening comment must start "// Moodle is distributed in the hope that it will be useful,".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 9 of the opening comment must start "// but WITHOUT ANY WARRANTY; without even the implied warranty of".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 10 of the opening comment must start "// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 11 of the opening comment must start "// GNU General Public License for more details.".
        
            $hasheading·=·($PAGE->heading);
        
            3: Line 12 of the opening comment must start "//".
        
            $hasnavbar·=·(empty($PAGE->layout_options['nonavbar'])·&&·$PAGE->has_navbar());
        
            4: Line 13 of the opening comment must start "// You should have received a copy of the GNU General Public License".
        
            $hasnavbar·=·(empty($PAGE->layout_options['nonavbar'])·&&·$PAGE->has_navbar());
        
            4: Line 14 of the opening comment must start "// along with Moodle. If not, see <http://www.gnu.org/licenses/>.".
        
            <?php
        
            1: End of line character is invalid; expected "\n" but found "\r\n"
        
            <?php·}·?>
        
            61: Line indented incorrectly; expected at least 4 spaces, found 0
        
            <?php·}·?>
        
            61: Closing brace must be on a line by itself
        
            ········<?php·if·($hasheading)·{·?>
        
            40: Line indented incorrectly; expected 4 spaces, found 0
        
            ········?></div><?php·}·?>
        
            50: Closing brace must be on a line by itself
        
            ············if·($haslogininfo)·{
        
            43: Line indented incorrectly; expected 8 spaces, found 12
        
            ············if·(!empty($PAGE->layout_options['langmenu']))·{
        
            46: Line indented incorrectly; expected 8 spaces, found 12
        
            ········<?php·if·($hascustommenu)·{·?>
        
            51: Line indented incorrectly; expected 4 spaces, found 0
        
            ········<?php·}·?>
        
            53: Closing brace must be on a line by itself
        
            ········<?php·if·($hasnavbar)·{·?>
        
            54: Line indented incorrectly; expected 4 spaces, found 0
        
            ········<?php·}·?>
        
            59: Closing brace must be on a line by itself
        
            ················<?php·}·?>
        
            82: Closing brace must be on a line by itself
        
            ················<?php·}·?>
        
            90: Closing brace must be on a line by itself
        
            ····<?php·}·?>
        
            105: Closing brace must be on a line by itself
        
            $hassidepre·=·(empty($PAGE->layout_options['noblocks'])·&&·$PAGE->blocks->region_has_content('side-pre',·$OUTPUT));
        
            6: Line exceeds 100 characters; contains 115 characters
        
            $hassidepost·=·(empty($PAGE->layout_options['noblocks'])·&&·$PAGE->blocks->region_has_content('side-post',·$OUTPUT));
        
            7: Line exceeds 100 characters; contains 117 characters
        
            <body·id="<?php·p($PAGE->bodyid)·?>"·class="<?php·p($PAGE->bodyclasses.'·'.join('·',·$bodyclasses))·?>">
        
            35: Line exceeds 100 characters; contains 104 characters
            
           

        If theme/base/layout/general.php is wrong how can we begin to get things right to start building or updating a CORE theme?

        Show
        Mary Evans added a comment - - edited @Eloy Forget the Peer Review, as I feel I am in a "NO WIN" situation here. I have been using the Code Checker which works great...only thing is I don't understand some of the developer jargon. Anyway, I was trying to get to grips with it, and thought I would code check theme/base/layout/general.php as I was sure the code in THAT file would be Ok...but how wrong could I was! Here's the printout. Files found: 1 theme\base\layout\general.php - 28 error(s) and 3 warning(s) Total: 28 error(s) and 3 warning(s) theme\base\layout\general.php 2: Line 1 of the opening comment must start "// This file is part of". $hasheading·=·($PAGE->heading); 3: Line 2 of the opening comment must start "//". $hasheading·=·($PAGE->heading); 3: Line 3 of the opening comment must start "// Moodle is free software: you can redistribute it and/or modify". $hasheading·=·($PAGE->heading); 3: Line 4 of the opening comment must start "// it under the terms of the GNU General Public License as published by". $hasheading·=·($PAGE->heading); 3: Line 5 of the opening comment must start "// the Free Software Foundation, either version 3 of the License, or". $hasheading·=·($PAGE->heading); 3: Line 6 of the opening comment must start "// (at your option) any later version.". $hasheading·=·($PAGE->heading); 3: Line 7 of the opening comment must start "//". $hasheading·=·($PAGE->heading); 3: Line 8 of the opening comment must start "// Moodle is distributed in the hope that it will be useful,". $hasheading·=·($PAGE->heading); 3: Line 9 of the opening comment must start "// but WITHOUT ANY WARRANTY; without even the implied warranty of". $hasheading·=·($PAGE->heading); 3: Line 10 of the opening comment must start "// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the". $hasheading·=·($PAGE->heading); 3: Line 11 of the opening comment must start "// GNU General Public License for more details.". $hasheading·=·($PAGE->heading); 3: Line 12 of the opening comment must start "//". $hasnavbar·=·(empty($PAGE->layout_options['nonavbar'])·&&·$PAGE->has_navbar()); 4: Line 13 of the opening comment must start "// You should have received a copy of the GNU General Public License". $hasnavbar·=·(empty($PAGE->layout_options['nonavbar'])·&&·$PAGE->has_navbar()); 4: Line 14 of the opening comment must start "// along with Moodle. If not, see <http://www.gnu.org/licenses/>.". <?php 1: End of line character is invalid; expected "\n" but found "\r\n" <?php·}·?> 61: Line indented incorrectly; expected at least 4 spaces, found 0 <?php·}·?> 61: Closing brace must be on a line by itself ········<?php·if·($hasheading)·{·?> 40: Line indented incorrectly; expected 4 spaces, found 0 ········?></div><?php·}·?> 50: Closing brace must be on a line by itself ············if·($haslogininfo)·{ 43: Line indented incorrectly; expected 8 spaces, found 12 ············if·(!empty($PAGE->layout_options['langmenu']))·{ 46: Line indented incorrectly; expected 8 spaces, found 12 ········<?php·if·($hascustommenu)·{·?> 51: Line indented incorrectly; expected 4 spaces, found 0 ········<?php·}·?> 53: Closing brace must be on a line by itself ········<?php·if·($hasnavbar)·{·?> 54: Line indented incorrectly; expected 4 spaces, found 0 ········<?php·}·?> 59: Closing brace must be on a line by itself ················<?php·}·?> 82: Closing brace must be on a line by itself ················<?php·}·?> 90: Closing brace must be on a line by itself ····<?php·}·?> 105: Closing brace must be on a line by itself $hassidepre·=·(empty($PAGE->layout_options['noblocks'])·&&·$PAGE->blocks->region_has_content('side-pre',·$OUTPUT)); 6: Line exceeds 100 characters; contains 115 characters $hassidepost·=·(empty($PAGE->layout_options['noblocks'])·&&·$PAGE->blocks->region_has_content('side-post',·$OUTPUT)); 7: Line exceeds 100 characters; contains 117 characters <body·id="<?php·p($PAGE->bodyid)·?>"·class="<?php·p($PAGE->bodyclasses.'·'.join('·',·$bodyclasses))·?>"> 35: Line exceeds 100 characters; contains 104 characters If theme/base/layout/general.php is wrong how can we begin to get things right to start building or updating a CORE theme?
        Hide
        Mary Evans added a comment -

        @Eloy,

        Success! I found what the problem was.

        After instructions from Martin some months ago I had set my TextPad (editor) to save CSS files as UNIX and UTF-8, however, I forgot to do the same with PHP files, so they were defaulting back to PC mode!

        I fixed that last night and all the files except general.php & frontpage.php are all tidy and got the Well Done! in the Code Checker...so I feel I am getting somewhere at last.

        Show
        Mary Evans added a comment - @Eloy, Success! I found what the problem was. After instructions from Martin some months ago I had set my TextPad (editor) to save CSS files as UNIX and UTF-8, however, I forgot to do the same with PHP files, so they were defaulting back to PC mode! I fixed that last night and all the files except general.php & frontpage.php are all tidy and got the Well Done! in the Code Checker...so I feel I am getting somewhere at last.
        Hide
        Mary Evans added a comment -

        @Caroline

        After fixing all the problems with the files, I did some hard testing and debugging and found that there were some strings missing, also the Code Checker queried some code in the settings.php, looks like you commented out some code to do with setting region widths in the sideblocks.

        I fixed the strings, but would like to know why you commented out the code in the first place. If it is not needed, then it need not be left in, and all the related code in the lib.php can be deleted too.

        Can you give me some clarification on this?

        Show
        Mary Evans added a comment - @Caroline After fixing all the problems with the files, I did some hard testing and debugging and found that there were some strings missing, also the Code Checker queried some code in the settings.php, looks like you commented out some code to do with setting region widths in the sideblocks. I fixed the strings, but would like to know why you commented out the code in the first place. If it is not needed, then it need not be left in, and all the related code in the lib.php can be deleted too. Can you give me some clarification on this?
        Hide
        Mary Evans added a comment - - edited

        I've just made an amendment, commited it and pushed it to github and then realised I forgot to rebase the branch...

        Or did I need to? Perhaps not as this was not on track for integration was it?

        Show
        Mary Evans added a comment - - edited I've just made an amendment, commited it and pushed it to github and then realised I forgot to rebase the branch... Or did I need to? Perhaps not as this was not on track for integration was it?
        Hide
        Mary Evans added a comment -

        I have just been looking at all the commits...this is looking really bad!

        Show
        Mary Evans added a comment - I have just been looking at all the commits...this is looking really bad!
        Hide
        Eloy Lafuente (stronk7) 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
        Eloy Lafuente (stronk7) 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 -

        Tried to rebase but ran into problems and so aborted operation.

        Show
        Mary Evans added a comment - Tried to rebase but ran into problems and so aborted operation.
        Hide
        Mary Evans added a comment -

        I decided to re-do this as the commits got all messed up. Trying to rebase looked like a nightmare so cut my losses and bailed out!

        Show
        Mary Evans added a comment - I decided to re-do this as the commits got all messed up. Trying to rebase looked like a nightmare so cut my losses and bailed out!
        Hide
        Mary Evans added a comment -

        This is a complete revamp of Splash Theme and so only needs adding to Master.

        I checked all php files using 'Code Checker' and sure enough there was enough white space to paint the White House! I have added the required documentation to those files that required it as directed by the Code Checker. All passed as correct.

        Thanks to Helen and Eloy for your valued support in this issue.

        Show
        Mary Evans added a comment - This is a complete revamp of Splash Theme and so only needs adding to Master. I checked all php files using 'Code Checker' and sure enough there was enough white space to paint the White House! I have added the required documentation to those files that required it as directed by the Code Checker. All passed as correct. Thanks to Helen and Eloy for your valued support in this issue.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Added one extra mini commit fixing some @package occurrences (must be theme_splash).

        Integrating...

        Show
        Eloy Lafuente (stronk7) added a comment - Added one extra mini commit fixing some @package occurrences (must be theme_splash). Integrating...
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Aparup Banerjee added a comment -
        • different pages and areas are looking good.
        • docking blocks , navigation , kyeyboard all seems fine.
        • custom settings working (i didn't test custom css tho)
        • design is VERY consistent , even black font saying "Hi Admin!" up on the black bar. (which is not good btw - but a separate issue)

        passes for me.

        Show
        Aparup Banerjee added a comment - different pages and areas are looking good. docking blocks , navigation , kyeyboard all seems fine. custom settings working (i didn't test custom css tho) design is VERY consistent , even black font saying "Hi Admin!" up on the black bar. (which is not good btw - but a separate issue) passes for me.
        Hide
        Aparup Banerjee added a comment -

        I've also added Barbara - she's got a good designer eye for design consistencies, i think she could churn out some issues.

        Show
        Aparup Banerjee added a comment - I've also added Barbara - she's got a good designer eye for design consistencies, i think she could churn out some issues.
        Hide
        Mary Evans added a comment -

        @Eloy
        Thanks!

        @Aparup
        Thanks for catching the "black on black" font colour issue in the header, I've added a sub-task to fix it.

        Show
        Mary Evans added a comment - @Eloy Thanks! @Aparup Thanks for catching the "black on black" font colour issue in the header, I've added a sub-task to fix it.
        Hide
        Barbara Ramiro added a comment -

        Hi Mary,

        First of all I like your design. It's artistic and abstract and yet clean. Well done! Anyway, Apu asked me to have a look and here is a list of what I noticed that might have been overlooked or intentional so please ignore it.

        ----- General -----

        1. The whole interface moves 30pixels to the left when changing theme colour from maroon to any colour with a docked block.

        ----- Header -----

        2. Long text or course title goes beyond the white box at the header.

        3. The text on the white box at the header for the yellow theme is colour grey instead of orange, same with available course titles under available courses and other hyperlinks unless its deliberately desaturated as compared to the other 3 theme colours.

        ----- Undocked blocks-----

        4. With a 2 liner block title, it overflows from the title background including the editing buttons. The background could be bigger and flexible to include the editing buttons as it gets pretty confusing whether the editing buttons are for the title or for the contents.

        5. Block editing buttons overflow from the title space when blocks are collapsed.

        6. Blue theme has 1pixel white line under block title bars as compared to the other 3 theme colours.

        7. After adding an image to the Main Menu block, it covers half of the main menu editing buttons.

        ----- Docked blocks -----

        8. Few editing buttons at the top right overflow to the next line for some docked blocks such as Latest News, Activities, Calendar etc. except for Navigation and Settings but after hovering at the Main Menu docked block all buttons get fixed.

        9. Would be nice to have the block titles left justified to make it look aligned.

        ----- Footer -----

        10. "Moodle Docs for this page" at the footer overlaps a bit on the body within the course page.

        11. The bottom part of the last topic/week is cropped after editing the number of weeks/topics.

        For any question or clarification please let me know.

        Cheers,
        Barbara

        Show
        Barbara Ramiro added a comment - Hi Mary, First of all I like your design. It's artistic and abstract and yet clean. Well done! Anyway, Apu asked me to have a look and here is a list of what I noticed that might have been overlooked or intentional so please ignore it. ----- General ----- 1. The whole interface moves 30pixels to the left when changing theme colour from maroon to any colour with a docked block. ----- Header ----- 2. Long text or course title goes beyond the white box at the header. 3. The text on the white box at the header for the yellow theme is colour grey instead of orange, same with available course titles under available courses and other hyperlinks unless its deliberately desaturated as compared to the other 3 theme colours. ----- Undocked blocks----- 4. With a 2 liner block title, it overflows from the title background including the editing buttons. The background could be bigger and flexible to include the editing buttons as it gets pretty confusing whether the editing buttons are for the title or for the contents. 5. Block editing buttons overflow from the title space when blocks are collapsed. 6. Blue theme has 1pixel white line under block title bars as compared to the other 3 theme colours. 7. After adding an image to the Main Menu block, it covers half of the main menu editing buttons. ----- Docked blocks ----- 8. Few editing buttons at the top right overflow to the next line for some docked blocks such as Latest News, Activities, Calendar etc. except for Navigation and Settings but after hovering at the Main Menu docked block all buttons get fixed. 9. Would be nice to have the block titles left justified to make it look aligned. ----- Footer ----- 10. "Moodle Docs for this page" at the footer overlaps a bit on the body within the course page. 11. The bottom part of the last topic/week is cropped after editing the number of weeks/topics. For any question or clarification please let me know. Cheers, Barbara
        Hide
        Mary Evans added a comment -

        Thanks Barbara,

        As this is not one of my themes, I was just getting it ready for integration, and streamlined some of the files to make it work better. I think many of the things you highlighted are a throwback from the way the theme was designed originally and almost 2 years old, and have have few changes since, until now.

        Thanks very much you your feedback...I'll pass this over to Caroline Kennedy who designed the theme and see if we can fix some of the things you highlighted.

        If you have time, we could do with more of this kind of feedback with all the CORE themes!

        Many thanks

        Mary

        Show
        Mary Evans added a comment - Thanks Barbara, As this is not one of my themes, I was just getting it ready for integration, and streamlined some of the files to make it work better. I think many of the things you highlighted are a throwback from the way the theme was designed originally and almost 2 years old, and have have few changes since, until now. Thanks very much you your feedback...I'll pass this over to Caroline Kennedy who designed the theme and see if we can fix some of the things you highlighted. If you have time, we could do with more of this kind of feedback with all the CORE themes! Many thanks Mary
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Well,

        I wish I said it every time
        you do the things you do.
        You always lend a helping hand,
        and I'm filled with gratitude.

        You are strong and generous
        for each and everyone one of us.
        I am eternally grateful,
        I cannot say thanks enough.

        Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Well, I wish I said it every time you do the things you do. You always lend a helping hand, and I'm filled with gratitude. You are strong and generous for each and everyone one of us. I am eternally grateful, I cannot say thanks enough. Sorry for the (un)cool bit above, lol. Closing this as fixed. Ciao
        Hide
        Mary Evans added a comment -

        Thanks

        Show
        Mary Evans added a comment - Thanks

          People

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

            Dates

            • Created:
              Updated:
              Resolved: