Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Themes
    • Testing Instructions:
      Hide

      NOTE FROM DAN: we need a bit of expository testing on this change in 2.5, to check for regressions with the following as a guideline:

      1. This change requires testing of a range of different page layouts using the clean theme and the standard theme (to check for regressions)
      2. For each of the following pages, check that the layout looks the same as before the patch, no javascript errors are thrown, and the block regions function correctly (including dragging and dropping blocks to empty regions).
        1. Front page
        2. Login page
        3. Course page
        4. Admin page(s)
        5. Page with an embedded iframe
        6. Safe browser page (quiz)
      3. Confirm behat is passing all tests.
      Show
      NOTE FROM DAN: we need a bit of expository testing on this change in 2.5, to check for regressions with the following as a guideline: This change requires testing of a range of different page layouts using the clean theme and the standard theme (to check for regressions) For each of the following pages, check that the layout looks the same as before the patch, no javascript errors are thrown, and the block regions function correctly (including dragging and dropping blocks to empty regions). Front page Login page Course page Admin page(s) Page with an embedded iframe Safe browser page (quiz) Confirm behat is passing all tests.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull 2.5 Branch:
      wip-MDL-39824-m25
    • Pull Master Branch:
      wip-MDL-39824-m26
    • Rank:
      47784

      Description

      This is a collection of improvements raised in a chat. The general goal is to simplify the themes by removing logic from the layout files.

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          +1 from me as for once this sounds logical!

          Show
          Mary Evans added a comment - +1 from me as for once this sounds logical!
          Hide
          Mary Evans added a comment -

          Adding watchers so they can vote for this initiative.

          Show
          Mary Evans added a comment - Adding watchers so they can vote for this initiative.
          Hide
          Julian Ridden added a comment -

          Thanks for adding me Mary. Also my +1

          Show
          Julian Ridden added a comment - Thanks for adding me Mary. Also my +1
          Hide
          Shaun Daubney added a comment -

          Thanks for adding me Mary

          Show
          Shaun Daubney added a comment - Thanks for adding me Mary
          Hide
          Gareth J Barnard added a comment -

          +1 for improvements & Mary Evans adding me. What's the plan?

          Show
          Gareth J Barnard added a comment - +1 for improvements & Mary Evans adding me. What's the plan?
          Hide
          Damyon Wiese added a comment -

          I'll add subtasks to this now (just didn't get to it on friday) - we can vote/discuss them individually.

          Show
          Damyon Wiese added a comment - I'll add subtasks to this now (just didn't get to it on friday) - we can vote/discuss them individually.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've a concept patch that shows how I'd like go about simplifying themes and at the same time getting block dnd working regardless of region etc.
          The patch attempts to remove as much logic as possible from theme layouts, but in turn introduces the requirements for more layout. I think this is a worthwhile move anyway.
          When printing block regions the core renderer now has a method that renders the basic structure for a block region. This predictability + some smart changes to the block dnd code allows that to get working, and I think makes the layouts a little simpler again.
          Also worth noting is that I've tried to remove all global variable uses except for $OUTPUT from the layout files.

          As part of this patch I have included a demo theme - multiregiontest that overrides just the course page layout and uses a total of 9 block regions.
          It is overkill but if good for testing multi-region functionality as well as showing what sort of styling is possible after this change.
          It also has an additional renderer method to show how you can override the blocks render method in order to create your own structure and serves to test altering the structure.

          If anyone has a moment to have a look at this and share there thoughts that would be most appreciated.
          Next stop will be the theme's forum if required.

          repo: git://github.com/samhemelryk/moodle.git
          branch: wip-MDL-39824-m26-concept
          diff: https://github.com/samhemelryk/moodle/commit/wip-MDL-39824-m26-concept

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've a concept patch that shows how I'd like go about simplifying themes and at the same time getting block dnd working regardless of region etc. The patch attempts to remove as much logic as possible from theme layouts, but in turn introduces the requirements for more layout. I think this is a worthwhile move anyway. When printing block regions the core renderer now has a method that renders the basic structure for a block region. This predictability + some smart changes to the block dnd code allows that to get working, and I think makes the layouts a little simpler again. Also worth noting is that I've tried to remove all global variable uses except for $OUTPUT from the layout files. As part of this patch I have included a demo theme - multiregiontest that overrides just the course page layout and uses a total of 9 block regions. It is overkill but if good for testing multi-region functionality as well as showing what sort of styling is possible after this change. It also has an additional renderer method to show how you can override the blocks render method in order to create your own structure and serves to test altering the structure. If anyone has a moment to have a look at this and share there thoughts that would be most appreciated. Next stop will be the theme's forum if required. repo: git://github.com/samhemelryk/moodle.git branch: wip- MDL-39824 -m26-concept diff: https://github.com/samhemelryk/moodle/commit/wip-MDL-39824-m26-concept Many thanks Sam
          Hide
          Julian Ridden added a comment -

          You can't give without a little take.

          I am far from the expert coder, but I do like the direction of Sam's patch. I will test more with it and give more detailed feedback. But certainly +1 on theoretical design methodology proposed.

          Naive question, I don't see this patch impacting Bootstrapbase development. Am I right there?

          Julian

          Show
          Julian Ridden added a comment - You can't give without a little take. I am far from the expert coder, but I do like the direction of Sam's patch. I will test more with it and give more detailed feedback. But certainly +1 on theoretical design methodology proposed. Naive question, I don't see this patch impacting Bootstrapbase development. Am I right there? Julian
          Hide
          Sam Hemelryk added a comment -

          Hi Julian,

          Thanks for taking the time to look at it.

          I'm not too sure what you're asking in regards to bootstrapbase development sorry, I'm think you mean will it impact on going work on the bootstrapbase theme?
          This patch pretty heftily changes the layouts, in fact general.php has been deleted and we have 5 more specific layouts now.
          However despite what would appear to be a lot of changes nothing about the actual HTML structure being used has changed. The patch is primarily focused on simplification, and I've resisted the urge to tweak as I know others have specific tasks for specific issues.
          The CSS I've added for the layouts is minimal and very easy to grasp. I have built a dependency here as the bootstrapbase theme now inherits bootstraps variable file as part of its less structure. That however should have no effect on the frontend design either.
          The goal of this issue is to no change the visual appearance of the themes at all but instead focus entirely on that idea of simplifying things, and getting the block dnd working finally.

          Hope that answers your question Julian, feel free to come back to me with more questions - no probs there at all.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Julian, Thanks for taking the time to look at it. I'm not too sure what you're asking in regards to bootstrapbase development sorry, I'm think you mean will it impact on going work on the bootstrapbase theme? This patch pretty heftily changes the layouts, in fact general.php has been deleted and we have 5 more specific layouts now. However despite what would appear to be a lot of changes nothing about the actual HTML structure being used has changed. The patch is primarily focused on simplification, and I've resisted the urge to tweak as I know others have specific tasks for specific issues. The CSS I've added for the layouts is minimal and very easy to grasp. I have built a dependency here as the bootstrapbase theme now inherits bootstraps variable file as part of its less structure. That however should have no effect on the frontend design either. The goal of this issue is to no change the visual appearance of the themes at all but instead focus entirely on that idea of simplifying things, and getting the block dnd working finally. Hope that answers your question Julian, feel free to come back to me with more questions - no probs there at all. Many thanks Sam
          Hide
          Richard Oelmann added a comment -

          Downloading from your git repository now Sam Love the plan to get the blocks back working wherever they are, and the simplification of the logic in the themes.
          Hoping (and will test) that this will work with themes based on standard as well as those based on bootstrapbase.
          Will it enable the dock to be used alongside the drag and drop of blocks?
          Drag and drop of blocks into multiple regions and the continuing availability of the dock are, for me, the two 'biggies' at the moment - without breaking existing themes if possible!
          Richard

          Show
          Richard Oelmann added a comment - Downloading from your git repository now Sam Love the plan to get the blocks back working wherever they are, and the simplification of the logic in the themes. Hoping (and will test) that this will work with themes based on standard as well as those based on bootstrapbase. Will it enable the dock to be used alongside the drag and drop of blocks? Drag and drop of blocks into multiple regions and the continuing availability of the dock are, for me, the two 'biggies' at the moment - without breaking existing themes if possible! Richard
          Hide
          Mary Evans added a comment - - edited

          Thanks Sam...I knew you would get round to this problem sooner or later.

          PS: I LOVE the color GREEN! Yeah!

          Show
          Mary Evans added a comment - - edited Thanks Sam...I knew you would get round to this problem sooner or later. PS: I LOVE the color GREEN! Yeah!
          Hide
          Stuart Lamour added a comment -

          hi all, did some tidy work a while back on clean including :

          simplifying l2r/r2l layouts
          reducing code duplication
          adding layout variations (blocks all left, all right etc)

          code is all still here https://github.com/stuartlamour/moodle/blob/master/theme/clean/

          might be good to add some of this to your patch sam?

          Show
          Stuart Lamour added a comment - hi all, did some tidy work a while back on clean including : simplifying l2r/r2l layouts reducing code duplication adding layout variations (blocks all left, all right etc) code is all still here https://github.com/stuartlamour/moodle/blob/master/theme/clean/ might be good to add some of this to your patch sam?
          Hide
          Mary Evans added a comment -

          @Sam: The branch you based this on need rebasing as it is conflicting with current Moodle(master). Error message just now says the code is older than the database on my current localhost/moodle26 install. So in short I can't see your changes.

          There are three lines of white space errors and it conficts with bootstrapbas as this has had a few fixes last week.

          Show
          Mary Evans added a comment - @Sam: The branch you based this on need rebasing as it is conflicting with current Moodle(master). Error message just now says the code is older than the database on my current localhost/moodle26 install. So in short I can't see your changes. There are three lines of white space errors and it conficts with bootstrapbas as this has had a few fixes last week.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've rebased the patch and pushed it up again. Those issues you were hitting have been resolved thanks Mary.
          Richard the changes are backwards compatabile in that I've introduced a couple of new $OUTPUT api calls that themes can use.
          These new API methods fix up a few structure issues, deal with rtl swtiching internally, and add a couple of new classes to structure automatically.
          The new classes are used by the dnd code and when it executes it decides whether to run the old busted up version of dnd or the new version that relies on those classes and should work for all themes that upgrade to the new API calls.
          Really all this means is that any theme converted to use these new methods will benefit and old themes not yet converted will continue to work exactly as they did before.
          In the future we'll be able to easily drop the old dnd code, and deprecate the old OUTPUT methods. However for the time being they run side by side and people can upgrade at there leisure.
          As for the dock that is another story, the dock has always been a very alternative UI component. How it fits into these bootstrap themes probably deserves its own dedicate issue.
          I'll create an issue for that shortly if one doesn't already exist.

          Stuart, this patch only cleans up the bootstrapbase theme at the moment however it will definitely be applied to clean and to a more limited degree the base/standard themes as well.
          Do you have issues in tracker for the changes you've made yet? If so then I'll look there otherwise I'll have a look at your branch and see what I can swing.

          Show
          Sam Hemelryk added a comment - Hi guys, I've rebased the patch and pushed it up again. Those issues you were hitting have been resolved thanks Mary. Richard the changes are backwards compatabile in that I've introduced a couple of new $OUTPUT api calls that themes can use. These new API methods fix up a few structure issues, deal with rtl swtiching internally, and add a couple of new classes to structure automatically. The new classes are used by the dnd code and when it executes it decides whether to run the old busted up version of dnd or the new version that relies on those classes and should work for all themes that upgrade to the new API calls. Really all this means is that any theme converted to use these new methods will benefit and old themes not yet converted will continue to work exactly as they did before. In the future we'll be able to easily drop the old dnd code, and deprecate the old OUTPUT methods. However for the time being they run side by side and people can upgrade at there leisure. As for the dock that is another story, the dock has always been a very alternative UI component. How it fits into these bootstrap themes probably deserves its own dedicate issue. I'll create an issue for that shortly if one doesn't already exist. Stuart, this patch only cleans up the bootstrapbase theme at the moment however it will definitely be applied to clean and to a more limited degree the base/standard themes as well. Do you have issues in tracker for the changes you've made yet? If so then I'll look there otherwise I'll have a look at your branch and see what I can swing.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Sam Hemelryk,

          I've read through your patch with interest and its purpose in making themes simpler. I've also been reading about 'Tiles' templating framework - http://viralpatel.net/blogs/spring-3-mvc-tiles-plugin-tutorial-example-eclipse/ - which is an Apache project http://tiles.apache.org/index.html. With the 'Tiles' concept the page is broken down into many constituent fragments for reuse in pages. The idea is that these 'fragments' can then be defined once and used over and over again within many pages providing a consistent look and reduced maintenance. Then when I was looking at the many new layouts for the columns (1-3) and realised that there is repetition of the same sort of thing like headers. So, what if Moodle themes could do the same thing? Perhaps with 'base' layout 'tiles' which then sibling themes could use.

          So, such an implementation could take the form of a 'tiles' sub-folder of 'layouts' folder with a tile of say 'header.php' containing:

          <head>
              <title><?php echo $OUTPUT->page_title(); ?></title>
              <link rel="shortcut icon" href="<?php echo $OUTPUT->favicon(); ?>" />
              <?php echo $OUTPUT->standard_head_html() ?>
              <meta name="viewport" content="width=device-width, initial-scale=1.0">
          </head>
          

          then the main layout file would have:

          <?php
          // This file is part of Moodle - http://moodle.org/
          // ....
          
          echo $OUTPUT->doctype() 
          
          $theme->includetile('header');
          
          ?>
          <body <?php echo $OUTPUT->body_attributes(); ?>>
          
          <?php echo $OUTPUT->standard_top_of_body_html() 
          
          $theme->includetile('navigation');
          ?>
          
          <div id="page" class="container-fluid">
          
          <?php $theme->includetile('page-header'); ?>
          
              <div id="page-content">
                  <?php $theme->includetile('multiblocktest-columns'); ?>
              </div>
          
          <?php $theme->includetile('footer'); ?>
          
          </div>
          </body>
          </html>
          

          then the 'config.php' file would have...

          <?php
          
          $THEME->name = 'multiblocktest';
          $THEME->parents = array('bootstrapbase');
          $THEME->tiles = array('multiblocktest-columns');
          $THEME->parenttiles = array('header', 'navigation', 'page-header', 'footer');
          $THEME->doctype = 'html5';
          ... etc.
          

          Thoughts?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Sam Hemelryk , I've read through your patch with interest and its purpose in making themes simpler. I've also been reading about 'Tiles' templating framework - http://viralpatel.net/blogs/spring-3-mvc-tiles-plugin-tutorial-example-eclipse/ - which is an Apache project http://tiles.apache.org/index.html . With the 'Tiles' concept the page is broken down into many constituent fragments for reuse in pages. The idea is that these 'fragments' can then be defined once and used over and over again within many pages providing a consistent look and reduced maintenance. Then when I was looking at the many new layouts for the columns (1-3) and realised that there is repetition of the same sort of thing like headers. So, what if Moodle themes could do the same thing? Perhaps with 'base' layout 'tiles' which then sibling themes could use. So, such an implementation could take the form of a 'tiles' sub-folder of 'layouts' folder with a tile of say 'header.php' containing: <head> <title><?php echo $OUTPUT->page_title(); ?></title> <link rel= "shortcut icon" href= "<?php echo $OUTPUT->favicon(); ?>" /> <?php echo $OUTPUT->standard_head_html() ?> <meta name= "viewport" content= "width=device-width, initial-scale=1.0" > </head> then the main layout file would have: <?php // This file is part of Moodle - http://moodle.org/ // .... echo $OUTPUT->doctype() $theme->includetile('header'); ?> <body <?php echo $OUTPUT->body_attributes(); ?>> <?php echo $OUTPUT->standard_top_of_body_html() $theme->includetile('navigation'); ?> <div id= "page" class= "container-fluid" > <?php $theme->includetile('page-header'); ?> <div id= "page-content" > <?php $theme->includetile('multiblocktest-columns'); ?> </div> <?php $theme->includetile('footer'); ?> </div> </body> </html> then the 'config.php' file would have... <?php $THEME->name = 'multiblocktest'; $THEME->parents = array('bootstrapbase'); $THEME->tiles = array('multiblocktest-columns'); $THEME->parenttiles = array('header', 'navigation', 'page-header', 'footer'); $THEME->doctype = 'html5'; ... etc. Thoughts? Cheers, Gareth
          Hide
          Richard Oelmann added a comment -

          Hi Gareth,
          I agree that much of what is int he layout files is repetitive and could be taken out into component subsections for repeated use (particularly where multiple layout files are used in one theme), but isn't this just an extension of what we can already do (and some themes do do) in terms of using the include('somefile.php') - e.g. some of the themes with profilebars, separate menu structures such as the awesomebar, or in Zebra with the footer.
          Is there a need to bring separate structures such as tiles in to do this? If the 'tiles' exist in the parent theme, could you use include('./parentthemename/layout/somefile.php') to reuse them if required? What are the advantages of tiles and using the config file to declare them over using include? - Asking for the sake of learning and information
          Richard

          Show
          Richard Oelmann added a comment - Hi Gareth, I agree that much of what is int he layout files is repetitive and could be taken out into component subsections for repeated use (particularly where multiple layout files are used in one theme), but isn't this just an extension of what we can already do (and some themes do do) in terms of using the include('somefile.php') - e.g. some of the themes with profilebars, separate menu structures such as the awesomebar, or in Zebra with the footer. Is there a need to bring separate structures such as tiles in to do this? If the 'tiles' exist in the parent theme, could you use include('./parentthemename/layout/somefile.php') to reuse them if required? What are the advantages of tiles and using the config file to declare them over using include? - Asking for the sake of learning and information Richard
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Richard Oelmann,

          I agree that "include('somefile.php')" is simpler. But then it's loose in a way that everybody can do their own thing to their own design leading to a reduction in reuse because of no defined standard - many nuts and bolts but none of them can be changed between themselves. I think that the people who perceived the idea to implement 'Tiles' thought the same way as just use 'include...' as I'm sure there would be a way of having inclusions on 'jsp' pages, but they went ahead and created it anyway. I think that having a defined structural standard will provide stability such that 'fragment' reuse across themes of different authors will lead to greater confidence and code reuse. For example, it is possible to include the CSS manually but then we use '$THEME->parentsheets'. So why not for reusable fragments of layouts?

          My thought on using 'config.php' falls in line with the rest where CSS sheets, JavaScript, Renderers etc. are all declared this way.

          It's just a thought based upon an observation, reading up on different stuff and thinking about 'Design Patterns' and 'Refactoring' having been lucky enough to have been to tutorials by John Vlissides and Martin Fowler.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Richard Oelmann , I agree that "include('somefile.php')" is simpler. But then it's loose in a way that everybody can do their own thing to their own design leading to a reduction in reuse because of no defined standard - many nuts and bolts but none of them can be changed between themselves. I think that the people who perceived the idea to implement 'Tiles' thought the same way as just use 'include...' as I'm sure there would be a way of having inclusions on 'jsp' pages, but they went ahead and created it anyway. I think that having a defined structural standard will provide stability such that 'fragment' reuse across themes of different authors will lead to greater confidence and code reuse. For example, it is possible to include the CSS manually but then we use '$THEME->parentsheets'. So why not for reusable fragments of layouts? My thought on using 'config.php' falls in line with the rest where CSS sheets, JavaScript, Renderers etc. are all declared this way. It's just a thought based upon an observation, reading up on different stuff and thinking about 'Design Patterns' and 'Refactoring' having been lucky enough to have been to tutorials by John Vlissides and Martin Fowler. Cheers, Gareth
          Hide
          Mary Evans added a comment -

          @Sam, I'm still getting conflicts with the code. According to your GitHub your master branch is 16 days old. Should you not have based this on current master for people to review/test? Or is the method of...

          git fetch git://github.com/samhemelryk/moodle.git wip-MDL-39824-m26-concept

          ...and then creating a branch on my localhost master branch with...

          git checkout -b MDL-39824_simples FETCH_HEAD

          ...the wrong way to go about peer reviewing these changes?

          Show
          Mary Evans added a comment - @Sam, I'm still getting conflicts with the code. According to your GitHub your master branch is 16 days old. Should you not have based this on current master for people to review/test? Or is the method of... git fetch git://github.com/samhemelryk/moodle.git wip-MDL-39824-m26-concept ...and then creating a branch on my localhost master branch with... git checkout -b MDL-39824_simples FETCH_HEAD ...the wrong way to go about peer reviewing these changes?
          Hide
          Mary Evans added a comment -

          Fixed it...thanks. Just needed rebasing this end too.

          Show
          Mary Evans added a comment - Fixed it...thanks. Just needed rebasing this end too.
          Hide
          Mary Evans added a comment -

          @Sam: So refreshing to find you too are human!

          Invalid get_string() identifier: 'pluginname' or component 'theme_multiblocktest'. Perhaps you are missing $string['pluginname'] = ''; in C:\wamp\www\moodle26/theme/multiblocktest/lang/en/theme_multiblocktest.php?

          line 6909 of \lib\moodlelib.php: call to debugging()
          line 7601 of \lib\moodlelib.php: call to core_string_manager->get_string()
          line 175 of \theme\index.php: call to get_string()

          Show
          Mary Evans added a comment - @Sam: So refreshing to find you too are human! Invalid get_string() identifier: 'pluginname' or component 'theme_multiblocktest'. Perhaps you are missing $string ['pluginname'] = ''; in C:\wamp\www\moodle26/theme/multiblocktest/lang/en/theme_multiblocktest.php? line 6909 of \lib\moodlelib.php: call to debugging() line 7601 of \lib\moodlelib.php: call to core_string_manager->get_string() line 175 of \theme\index.php: call to get_string()
          Hide
          Mary Evans added a comment -

          For anyone else testing Sams work, there is a known Moodle bug MDL-39870 which throws up an error when editing is turned on in a course page. I reported this on Tuesday and Marina is fixing it.

          Show
          Mary Evans added a comment - For anyone else testing Sams work, there is a known Moodle bug MDL-39870 which throws up an error when editing is turned on in a course page. I reported this on Tuesday and Marina is fixing it.
          Hide
          Sam Hemelryk added a comment -

          Hiya,

          First up tiles.
          I always wonder what Moodle would be like if we had chosen to go down the road of templates rather than renderers.
          While working on this clean up I had a similiar feeling about the re-use of those components, the header and footer are excellent examples as with the layout files split those areas are common to most.
          Truthfully I dismissed it with the same thinking Richard applied, that it could easily be done anyway and my focus on this issue was in simplifying things as much as possible. While I feel that more specific layouts are easier to understand than a single generic layout with lots of logic to make it different I am not sure that having includes is a step in the same keep it simple stupid direction I was taking.

          However after reading through the comments between yourself Gareth and Richard I am honestly now of the opinion that at least facilitating it would be benefical.
          As mentioned facilitating it would aid in standardising how people do it and help ensure that things are both easily reusable between themes, and would help us in managing any further changes to core themes that would in turn no doubt affect custom themes overriding those core themes.
          Certainly such an improvement should be a separate task as well, I want to be sure that this simplification lands and as my integration leave ends today I want to wrap up the development as much as possible and get it up for peer-review.
          Damyon I believe will take over this issue from me as I'm back on integration and he will be coming on integration leave.

          Mary, yup missed the strings.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hiya, First up tiles. I always wonder what Moodle would be like if we had chosen to go down the road of templates rather than renderers. While working on this clean up I had a similiar feeling about the re-use of those components, the header and footer are excellent examples as with the layout files split those areas are common to most. Truthfully I dismissed it with the same thinking Richard applied, that it could easily be done anyway and my focus on this issue was in simplifying things as much as possible. While I feel that more specific layouts are easier to understand than a single generic layout with lots of logic to make it different I am not sure that having includes is a step in the same keep it simple stupid direction I was taking. However after reading through the comments between yourself Gareth and Richard I am honestly now of the opinion that at least facilitating it would be benefical. As mentioned facilitating it would aid in standardising how people do it and help ensure that things are both easily reusable between themes, and would help us in managing any further changes to core themes that would in turn no doubt affect custom themes overriding those core themes. Certainly such an improvement should be a separate task as well, I want to be sure that this simplification lands and as my integration leave ends today I want to wrap up the development as much as possible and get it up for peer-review. Damyon I believe will take over this issue from me as I'm back on integration and he will be coming on integration leave. Mary, yup missed the strings. Cheers Sam
          Hide
          Damyon Wiese added a comment -

          My only concern about moving to the use of includes/tiles in the layout files is one that the theme designers could comment on.

          As it is now, a junior web developer could just about load a single layout file in dreamweaver (or whatever the cool kids use these days) and because it is almost HTML - they could just make some changes and save it - not touching anything they don't understand.

          Moving to files that are not self contained, means they would need to hunt to find the header, hunt to find the footer and each would only be a html fragment - so they couldn't see it visually in the context of the page. The trade off is they would only have to make their changes once, and not 5 or 6 times.

          Show
          Damyon Wiese added a comment - My only concern about moving to the use of includes/tiles in the layout files is one that the theme designers could comment on. As it is now, a junior web developer could just about load a single layout file in dreamweaver (or whatever the cool kids use these days) and because it is almost HTML - they could just make some changes and save it - not touching anything they don't understand. Moving to files that are not self contained, means they would need to hunt to find the header, hunt to find the footer and each would only be a html fragment - so they couldn't see it visually in the context of the page. The trade off is they would only have to make their changes once, and not 5 or 6 times.
          Hide
          Richard Oelmann added a comment -

          Hi Damyon,
          I'm personally in two minds about this -
          I can see the advantages of the Tiles suggestion and being able to reuse them from parent themes, in much the same way that entire layout files can be reused currently if required.
          But at the same time, includes seem to me to do exactly the same thing and are a standard part of php that most theme designers (even those starting out) would be able to see and use, being a fairly obvious, self-explanatory tool. As such these are already supported (and used in some instances). Making them a more common feature of theme design in constructing the layouts would be a straightforward change for those wanting to use them and model them for others to follow, and once we start using them more commonly could be documented in the Theme docs as 'good practice'.
          I also take Damyon's point about seeing the fragments in the context of the page and, even if includes are used, having those fragments in the theme rather than in a parent or grandparent theme, makes them much easier to track - and seems to fit better with the 'keep it simple' direction of Sam's work.
          Comments from other themers anticipated and welcomed

          Richard

          Show
          Richard Oelmann added a comment - Hi Damyon, I'm personally in two minds about this - I can see the advantages of the Tiles suggestion and being able to reuse them from parent themes, in much the same way that entire layout files can be reused currently if required. But at the same time, includes seem to me to do exactly the same thing and are a standard part of php that most theme designers (even those starting out) would be able to see and use, being a fairly obvious, self-explanatory tool. As such these are already supported (and used in some instances). Making them a more common feature of theme design in constructing the layouts would be a straightforward change for those wanting to use them and model them for others to follow, and once we start using them more commonly could be documented in the Theme docs as 'good practice'. I also take Damyon's point about seeing the fragments in the context of the page and, even if includes are used, having those fragments in the theme rather than in a parent or grandparent theme, makes them much easier to track - and seems to fit better with the 'keep it simple' direction of Sam's work. Comments from other themers anticipated and welcomed Richard
          Hide
          Stuart Lamour added a comment -

          not sure i've ever met a web developer - junior or otherwise - that uses dreamweaver! adobe edge might be a more likely candidate these days.

          tiles looks really similar to wordpress themeing.

          As wordpress is the most widely used cms on the web its probably a good pattern to follow.

          if you have a look at the github link i sent https://github.com/stuartlamour/moodle/tree/master/theme/clean it moves all the vars out to separate them from the template/html stuff.

          I did this as well as separate header and footer elements in the original bootstrap (https://github.com/stuartlamour/moodle_bootstrap/tree/8624c5208d5bc77eeb19c954b53a7cf46771fd34/bootstrap/layout) - but bas told me off

          Be very much in favour of reverting back to that separation and inheritance pattern

          Show
          Stuart Lamour added a comment - not sure i've ever met a web developer - junior or otherwise - that uses dreamweaver! adobe edge might be a more likely candidate these days. tiles looks really similar to wordpress themeing. As wordpress is the most widely used cms on the web its probably a good pattern to follow. if you have a look at the github link i sent https://github.com/stuartlamour/moodle/tree/master/theme/clean it moves all the vars out to separate them from the template/html stuff. I did this as well as separate header and footer elements in the original bootstrap ( https://github.com/stuartlamour/moodle_bootstrap/tree/8624c5208d5bc77eeb19c954b53a7cf46771fd34/bootstrap/layout ) - but bas told me off Be very much in favour of reverting back to that separation and inheritance pattern
          Hide
          Gareth J Barnard added a comment -

          Hi everyone ,

          Pragmatically the only time I've used Dreamweaver was to teach it at school to ages 14-18 and even then it was a nightmare with the layout etc. Frontpage was the same.

          The journey of a developer is one of learning from the simple to the complex. So, perhaps having a simple, effective but optional 'tiles' system would be a good bet. It does not have to be used. So as an example, Bootstrap Base could use it as it already has the more skillful 'less' technology (unless you just clone Clean and tinker with custom.css), but do not use it in the 'Standard' theme. Therefore there would be core examples of how to create themes for developers of different levels of experience. Plus provide a way for learning to take place through the provision of an upgrade path - 'inclusion' .

          The think here is 'capability'. It was said at iMoot (Martin?) that most people only used 20% of Moodle's functionality. But that's the power, it is there already if you need it for your situation. I think there are now 60 million users of Moodle, so even if 1% need a particular bit of it then it's still 600 thousand users - not an insignificant amount. And that's the thing I'm starting to love about the Moodle community is all the positive and varied technologies that go into making it work.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi everyone , Pragmatically the only time I've used Dreamweaver was to teach it at school to ages 14-18 and even then it was a nightmare with the layout etc. Frontpage was the same. The journey of a developer is one of learning from the simple to the complex. So, perhaps having a simple, effective but optional 'tiles' system would be a good bet. It does not have to be used. So as an example, Bootstrap Base could use it as it already has the more skillful 'less' technology (unless you just clone Clean and tinker with custom.css), but do not use it in the 'Standard' theme. Therefore there would be core examples of how to create themes for developers of different levels of experience. Plus provide a way for learning to take place through the provision of an upgrade path - 'inclusion' . The think here is 'capability'. It was said at iMoot (Martin?) that most people only used 20% of Moodle's functionality. But that's the power, it is there already if you need it for your situation. I think there are now 60 million users of Moodle, so even if 1% need a particular bit of it then it's still 600 thousand users - not an insignificant amount. And that's the thing I'm starting to love about the Moodle community is all the positive and varied technologies that go into making it work. Cheers, Gareth
          Hide
          Richard Oelmann added a comment -

          I'm afraid I still don't see either the need or the benefits for using tiles. It appears to add an extra level of complexity that is not needed as everything it achieves can already be done with a single php command in the layout file and no changes to the config.php - easy to understand, easy to use, easy to build on, easy to extract into a single layout file or break into component parts as needed/wanted.

          I don't think this is anything to do with a 'developer's journey from simple to complex' as much as adding an extra unnecessary level of complexity to design features and standing in the way of non-developers accessing the coding of themes. Not all theme developers are code programmers - or want to be.
          I do not think that dividing themes into 'skillful' or not is a helpful analogy to make either, or that the 'think' is capability - Martin's comments at the iMoot referred to usability and the features that are there - the number of blocks, activities, plugins that provide the potential power of moodle, not to the development of the code.
          ALL themes need to be accessible to those who want to take on the development work for their institution - from a Primary school needing a teacher to take on minor recolouring and rebranding through to a university with a development team who can recompile LESS from scratch and everything in between.
          To be turning around to a new moodle themer and say, 'well we advise you not to use that theme because you dont know enough yet', is not going to be helpful to the development of the community and is going to add pressure to people like myself in the forum community providing support for those new themers.

          If tiles were to simplify the use of a complex structure needed to utilise components then I could see the benefit. But to replace a single line standard php command (include) with multiple configuration settings that need to be tracked through parent/grandparent themes to find those components and how they are configured does not make any sense to me. It doesn't add additional functionality or power, just additional complexity.

          I say all this as someone who came into Moodle theming by the very route described above - a Primary school teacher who started finding out from the community how to recolour and rebrand a theme and have developed my own knowledge through that route in the community and as someone with zero development experience at that time. I had to learn HTML, PHP and CSS. I have to say that if I had been faced then with the complexity of things I am now seeing, as someone with a little more experience in the theme designs, I would have given up at that point - to be faced with having to learn about LESS in order to understand where the things I wanted to change were coming from, to understand more and more complex configurations that take me away from standard php/html/css is, to my mind, a block to access for the 'non-developer' entering the community.

          We already have a fracture point between standard themes and bootstrap themes, I think the suggestion that tiles should work for bootstrap but not for 'standard' themes is plain wrong. There should NOT be a split in themes!!! All functions should work for ALL themes - drag and drop of blocks, multiple block locations, the dock bar: these are all features which should be working by default for ALL themes. some of these features have been broken - and labelled as 'blockers' since Moodle2.3 and yet development has gone on in other directions ignoring these issues - or creating new ones!

          If it is decided to move to a tile like structure a-la Wordpress (why? Moodle doesn't need to try to become Wordpress/Twitter/Facebook/Blogger/Amazon or any other 'big thing') then we should be starting now to look at the way we want things to go for Moodle3 and not rushing more changes in for 2.6 just because other people are using them, the way I feel bootstrap has been rushed in to 2.5 because the rest of the web is using it. The ongoing discussions and issues around that seem to show that it was/is not fully ready - or Moodle is not ready for it! - and certainly not to replace base/standard as the default way forward. Bootstrap seems to me at the moment to be driving the way Moodle is being constructed, rather than providing a tool to create a skin/layout colour scheme/look and feel for moodle - is this the way it should be going? Undoubtedly, Moodle needs to move forward, but I feel we are in severe danger of breaking things (in the community if not code wise) by rushing ahead and creating these splits between 'this works if you have this kind of theme but not with that one'. The next thing will be course formats that only work if you have bootstrap themes, or tiles enabled.

          I am not adverse to progress and to moving forward - when things are mature and stable enough to do that - but I am adverse to adding 'unnecessary' features (in that I can see no additional functionality being provided or any simplification being created) that to my mind become roadblocks to entry into the community for people who want to be more than mere users, but are not and do not want to be full time developers.

          @Gareth - I think tiles as a whole concept needs to be thought VERY carefully before implementing for the sake of implementing
          @Sam/Damyon - please can we ensure that this simplification, and all other features, can be applied in ALL themes not just bootstrap ones. And can I also again make a plea for existing 'blocker' tracker issues to be fixed before we move even further down ANY road!!!

          Show
          Richard Oelmann added a comment - I'm afraid I still don't see either the need or the benefits for using tiles. It appears to add an extra level of complexity that is not needed as everything it achieves can already be done with a single php command in the layout file and no changes to the config.php - easy to understand, easy to use, easy to build on, easy to extract into a single layout file or break into component parts as needed/wanted. I don't think this is anything to do with a 'developer's journey from simple to complex' as much as adding an extra unnecessary level of complexity to design features and standing in the way of non-developers accessing the coding of themes. Not all theme developers are code programmers - or want to be. I do not think that dividing themes into 'skillful' or not is a helpful analogy to make either, or that the 'think' is capability - Martin's comments at the iMoot referred to usability and the features that are there - the number of blocks, activities, plugins that provide the potential power of moodle, not to the development of the code. ALL themes need to be accessible to those who want to take on the development work for their institution - from a Primary school needing a teacher to take on minor recolouring and rebranding through to a university with a development team who can recompile LESS from scratch and everything in between. To be turning around to a new moodle themer and say, 'well we advise you not to use that theme because you dont know enough yet', is not going to be helpful to the development of the community and is going to add pressure to people like myself in the forum community providing support for those new themers. If tiles were to simplify the use of a complex structure needed to utilise components then I could see the benefit. But to replace a single line standard php command (include) with multiple configuration settings that need to be tracked through parent/grandparent themes to find those components and how they are configured does not make any sense to me. It doesn't add additional functionality or power, just additional complexity. I say all this as someone who came into Moodle theming by the very route described above - a Primary school teacher who started finding out from the community how to recolour and rebrand a theme and have developed my own knowledge through that route in the community and as someone with zero development experience at that time. I had to learn HTML, PHP and CSS. I have to say that if I had been faced then with the complexity of things I am now seeing, as someone with a little more experience in the theme designs, I would have given up at that point - to be faced with having to learn about LESS in order to understand where the things I wanted to change were coming from, to understand more and more complex configurations that take me away from standard php/html/css is, to my mind, a block to access for the 'non-developer' entering the community. We already have a fracture point between standard themes and bootstrap themes, I think the suggestion that tiles should work for bootstrap but not for 'standard' themes is plain wrong. There should NOT be a split in themes!!! All functions should work for ALL themes - drag and drop of blocks, multiple block locations, the dock bar: these are all features which should be working by default for ALL themes. some of these features have been broken - and labelled as 'blockers' since Moodle2.3 and yet development has gone on in other directions ignoring these issues - or creating new ones! If it is decided to move to a tile like structure a-la Wordpress (why? Moodle doesn't need to try to become Wordpress/Twitter/Facebook/Blogger/Amazon or any other 'big thing') then we should be starting now to look at the way we want things to go for Moodle3 and not rushing more changes in for 2.6 just because other people are using them, the way I feel bootstrap has been rushed in to 2.5 because the rest of the web is using it. The ongoing discussions and issues around that seem to show that it was/is not fully ready - or Moodle is not ready for it! - and certainly not to replace base/standard as the default way forward. Bootstrap seems to me at the moment to be driving the way Moodle is being constructed, rather than providing a tool to create a skin/layout colour scheme/look and feel for moodle - is this the way it should be going? Undoubtedly, Moodle needs to move forward, but I feel we are in severe danger of breaking things (in the community if not code wise) by rushing ahead and creating these splits between 'this works if you have this kind of theme but not with that one'. The next thing will be course formats that only work if you have bootstrap themes, or tiles enabled. I am not adverse to progress and to moving forward - when things are mature and stable enough to do that - but I am adverse to adding 'unnecessary' features (in that I can see no additional functionality being provided or any simplification being created) that to my mind become roadblocks to entry into the community for people who want to be more than mere users, but are not and do not want to be full time developers. @Gareth - I think tiles as a whole concept needs to be thought VERY carefully before implementing for the sake of implementing @Sam/Damyon - please can we ensure that this simplification, and all other features, can be applied in ALL themes not just bootstrap ones. And can I also again make a plea for existing 'blocker' tracker issues to be fixed before we move even further down ANY road!!!
          Hide
          Mary Evans added a comment -

          Oh gosh...just reading this in between TV Ads!

          @Sam...your simplification looks good in Bootstrap, but breaks Clean theme.

          Night night all!

          Show
          Mary Evans added a comment - Oh gosh...just reading this in between TV Ads! @Sam...your simplification looks good in Bootstrap, but breaks Clean theme. Night night all!
          Hide
          Gareth J Barnard added a comment -

          Dear Richard Oelmann,

          I agree, it does need careful consideration. And 'tiles' is for all themes not just Bootstrap. I think that confusion is just down to the fact that the example code uses Bootstrap - it could have used the standard base - then we would be wondering about Bootstrap

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Richard Oelmann , I agree, it does need careful consideration. And 'tiles' is for all themes not just Bootstrap. I think that confusion is just down to the fact that the example code uses Bootstrap - it could have used the standard base - then we would be wondering about Bootstrap Cheers, Gareth
          Hide
          Richard Oelmann added a comment - - edited

          Gareth - it was your own comment above regarding applying tiles to bootstrapbase as already having a more skilfull technology ie LESS - a flawed idea in my opinion as I have posted elsewhere - that led to that comment, not the fact that Sam's simplification ideas are currently in a bootstrap theme - something which is not related to this new idea of Tiles which should I think be discussed entirely separately from Sam's simplification.
          Tiles are not a part of Sam's simplification work, they are an idea which you have raised and promoted in the thread and are not currently based on any theme other than through your previous comment.

          Show
          Richard Oelmann added a comment - - edited Gareth - it was your own comment above regarding applying tiles to bootstrapbase as already having a more skilfull technology ie LESS - a flawed idea in my opinion as I have posted elsewhere - that led to that comment, not the fact that Sam's simplification ideas are currently in a bootstrap theme - something which is not related to this new idea of Tiles which should I think be discussed entirely separately from Sam's simplification. Tiles are not a part of Sam's simplification work, they are an idea which you have raised and promoted in the thread and are not currently based on any theme other than through your previous comment.
          Hide
          Mary Evans added a comment - - edited

          @Sam,

          If Clean theme is to use the new layout files then they need to be either added to theme/clean/layout/ as Clean uses custom settings for logo and footer, or you could add layout files to theme/clean/layout that can be brought into theme/bootstrapbase/layout files using include() that points to theme/clean/layout/headerlogo.php and footnote.php, or use some of your PHP magic that does the same thing.

          Show
          Mary Evans added a comment - - edited @Sam, If Clean theme is to use the new layout files then they need to be either added to theme/clean/layout/ as Clean uses custom settings for logo and footer, or you could add layout files to theme/clean/layout that can be brought into theme/bootstrapbase/layout files using include() that points to theme/clean/layout/headerlogo.php and footnote.php, or use some of your PHP magic that does the same thing.
          Hide
          Gareth J Barnard added a comment -

          Richard Oelmann No problem.

          Show
          Gareth J Barnard added a comment - Richard Oelmann No problem.
          Hide
          Sam Hemelryk added a comment -

          Ok, putting my work up for peer-review.

          The set of patches I'm purposing address all the linked issues.
          Most notably they simplify the bootstrapbase and clean themes, in doing so they will break any theme's based upon either of those themes that do not reference their own layout files within the config.php file.
          I believe that bootstrapbase and clean were included as experimental themes so such breakage will be permitted.
          Left to do would be to convert all other core themes to use the parts of the new API that we would like them to use.
          I feel this is probably better left to a separate issue as no doubt there will be a line to draw in the sand about how much we convert given we may want to move away from them in the future.

          The tiles discussion has spun out quite a bit I see reading back (I've been off the past few days showing Andrew N around the top of the south island of NZ).
          Just to make a couple of things clear.
          This issue will not in any way move towards looking at tiles.
          Moodle will not be converting to a "tiled" theme system any time in the near future. Such a change would be huge and require reworking areas we have spent much time on already.
          I personally think the idea of facilitating a tile solution for themes so that they can separate out HTML and create reusable fragments in an organised way is a smart idea. If something happens in this area I do not think we would convert any core themes. It would have to be well documented and the "tile" support as a whole would be 100% optional.

          As for fixing blockers; At HQ teams (or team leaders) select issues to be worked upon during sprints. The sprints last 3 weeks and each team selects work proportionate to what they believe they can achieve in that time frame.
          There are a large number of components in Moodle, all with blockers no doubt. Priority is also only one factor that is looked at when selecting issues to work on, security status for instance is another, and popular issues (voting) as well.
          Worth noting is that we also give priority to issues with patches.
          It is up to the powers that be in selecting issues, unfortunately I can't promise any issue will get looked at. Being blockers hopefully they get onto our list sooner rather than later however it still a matter of resources over time.

          Show
          Sam Hemelryk added a comment - Ok, putting my work up for peer-review. The set of patches I'm purposing address all the linked issues. Most notably they simplify the bootstrapbase and clean themes, in doing so they will break any theme's based upon either of those themes that do not reference their own layout files within the config.php file. I believe that bootstrapbase and clean were included as experimental themes so such breakage will be permitted. Left to do would be to convert all other core themes to use the parts of the new API that we would like them to use. I feel this is probably better left to a separate issue as no doubt there will be a line to draw in the sand about how much we convert given we may want to move away from them in the future. The tiles discussion has spun out quite a bit I see reading back (I've been off the past few days showing Andrew N around the top of the south island of NZ). Just to make a couple of things clear. This issue will not in any way move towards looking at tiles. Moodle will not be converting to a "tiled" theme system any time in the near future. Such a change would be huge and require reworking areas we have spent much time on already. I personally think the idea of facilitating a tile solution for themes so that they can separate out HTML and create reusable fragments in an organised way is a smart idea. If something happens in this area I do not think we would convert any core themes. It would have to be well documented and the "tile" support as a whole would be 100% optional. As for fixing blockers; At HQ teams (or team leaders) select issues to be worked upon during sprints. The sprints last 3 weeks and each team selects work proportionate to what they believe they can achieve in that time frame. There are a large number of components in Moodle, all with blockers no doubt. Priority is also only one factor that is looked at when selecting issues to work on, security status for instance is another, and popular issues (voting) as well. Worth noting is that we also give priority to issues with patches. It is up to the powers that be in selecting issues, unfortunately I can't promise any issue will get looked at. Being blockers hopefully they get onto our list sooner rather than later however it still a matter of resources over time.
          Hide
          Richard Oelmann added a comment -

          Sam,
          I continue to be concerned about comments such as 'a line to draw in the sand about how much we convert given we may want to move away from them in the future.' and the implication of two-tier development for such excellent features as this simplification. This may be the long-term future, but if these developments are being added now, then surely they need to be available for all supported themes.
          If this is really the way MoodleHQ are planning - that we will move away from the existing themes at some point (presumably in the near future if discussions are already suggesting work in those themes should be limited from now) and that the support for the existing themes is already being talked of as less important, please can we have some direct guidance on this matter from HQ to the community.
          For as long as base/standard themes are supported in the core, I personally believe that all such development work should be available fully to these themes as well as to bootstrapbase themes. If that is not the way HQ sees it, please can the community be informed as soon as possible so we can target our own energies, time and resources appropriately.

          Show
          Richard Oelmann added a comment - Sam, I continue to be concerned about comments such as 'a line to draw in the sand about how much we convert given we may want to move away from them in the future.' and the implication of two-tier development for such excellent features as this simplification. This may be the long-term future, but if these developments are being added now, then surely they need to be available for all supported themes. If this is really the way MoodleHQ are planning - that we will move away from the existing themes at some point (presumably in the near future if discussions are already suggesting work in those themes should be limited from now) and that the support for the existing themes is already being talked of as less important, please can we have some direct guidance on this matter from HQ to the community. For as long as base/standard themes are supported in the core, I personally believe that all such development work should be available fully to these themes as well as to bootstrapbase themes. If that is not the way HQ sees it, please can the community be informed as soon as possible so we can target our own energies, time and resources appropriately.
          Hide
          Sam Hemelryk added a comment - - edited

          Hi Richard,
          These new things will be available to all themes, the question is simply whether we convert core themes as we will likely be deprecating them, also because it may cause regressions with child themes, and finally because we want to have time to make that decision (before 2.6 of course).
          Anyway, gotta run.
          Sam

          -edited because my English is horrific when I am in a hurry. Just to re-iterate this will be done before 2.6.

          Show
          Sam Hemelryk added a comment - - edited Hi Richard, These new things will be available to all themes, the question is simply whether we convert core themes as we will likely be deprecating them, also because it may cause regressions with child themes, and finally because we want to have time to make that decision (before 2.6 of course). Anyway, gotta run. Sam -edited because my English is horrific when I am in a hurry. Just to re-iterate this will be done before 2.6.
          Hide
          Richard Oelmann added a comment -

          Thanks for the clarification Sam

          Show
          Richard Oelmann added a comment - Thanks for the clarification Sam
          Hide
          Damyon Wiese added a comment -

          Yes Richard, the changes Sam has made to the renderers etc are available for any theme to use, but they don't have to. We'll make sure the docs get updated to document the new features.

          This patch implements the simplification only for bootstrapbase/clean because the direction for "clean" is that we are aiming for it to be the default theme for Moodle 2.6. We can add issues to do the same for other themes if there is demand, and they will get prioritized/voted on separately.

          Show
          Damyon Wiese added a comment - Yes Richard, the changes Sam has made to the renderers etc are available for any theme to use, but they don't have to. We'll make sure the docs get updated to document the new features. This patch implements the simplification only for bootstrapbase/clean because the direction for "clean" is that we are aiming for it to be the default theme for Moodle 2.6. We can add issues to do the same for other themes if there is demand, and they will get prioritized/voted on separately.
          Hide
          Richard Oelmann added a comment -

          Hi Damyon/Sam
          Its great to see this work being done and implemented for all themes
          I've seen so many posts recently in various forums/trackers with people seeming to suggest the imminent 'death' of the base/standard type themes and proposing two-tier development, I hope you can understand the concern expressed (not that the developments should necessarily be implemented into every core theme - possibly one or two as 'models'? - but that they should be available for those who want them, as you have clarified)- but I appreciate you have removed that concern in this instance Thank you.
          R

          Show
          Richard Oelmann added a comment - Hi Damyon/Sam Its great to see this work being done and implemented for all themes I've seen so many posts recently in various forums/trackers with people seeming to suggest the imminent 'death' of the base/standard type themes and proposing two-tier development, I hope you can understand the concern expressed (not that the developments should necessarily be implemented into every core theme - possibly one or two as 'models'? - but that they should be available for those who want them, as you have clarified)- but I appreciate you have removed that concern in this instance Thank you. R
          Hide
          Damyon Wiese added a comment -

          Note for integrators - this should be backported to 2.5 as there are no functionality changes and it will help us maintain bootstrap (and backport fixes) if they are in sync.

          Show
          Damyon Wiese added a comment - Note for integrators - this should be backported to 2.5 as there are no functionality changes and it will help us maintain bootstrap (and backport fixes) if they are in sync.
          Hide
          Damyon Wiese added a comment -

          Also note I saved the generated HTML for the standard theme with and without this patch and it was identical except for randomly generated ids.

          Show
          Damyon Wiese added a comment - Also note I saved the generated HTML for the standard theme with and without this patch and it was identical except for randomly generated ids.
          Hide
          Damyon Wiese added a comment -

          [Y] Syntax - The only thing i spotted here is some inline comments in the javascript that either had no fullstop or did not start with a captial (very minor)
          [Y] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing - (I added testing instructions for this)
          [-] Security
          [N] Documentation - (I added docs_required - it would be good to add some comments to themes/upgrade.txt as well)
          [Y] Git
          [Y] Sanity check (double yes!)

          I think it's worth adding the comments to themes/upgrade.txt but that is all that I can see that needs changing so feel free to submit this Sam.

          Thanks!

          Show
          Damyon Wiese added a comment - [Y] Syntax - The only thing i spotted here is some inline comments in the javascript that either had no fullstop or did not start with a captial (very minor) [Y] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing - (I added testing instructions for this) [-] Security [N] Documentation - (I added docs_required - it would be good to add some comments to themes/upgrade.txt as well) [Y] Git [Y] Sanity check (double yes!) I think it's worth adding the comments to themes/upgrade.txt but that is all that I can see that needs changing so feel free to submit this Sam. Thanks!
          Hide
          Mary Evans added a comment -

          Just testing, so far so good with normal screen resolutions, but responsive design needs working on some more...see attached image.

          Win7/FireFox21 (+ User Agent Switcher - set to iPhone3)

          Show
          Mary Evans added a comment - Just testing, so far so good with normal screen resolutions, but responsive design needs working on some more...see attached image. Win7/FireFox21 (+ User Agent Switcher - set to iPhone3)
          Hide
          Mary Evans added a comment - - edited

          Block region issue in RTL language

          Same problem as MDL-40067

          Show
          Mary Evans added a comment - - edited Block region issue in RTL language Same problem as MDL-40067
          Hide
          Sam Hemelryk added a comment -

          Thanks Damyon,

          I've added two additional commits, the first to fix the RTL dnd issue Mary linked to that was still present in my changes (easy fix) and the second to add details to theme/upgrade.txt.
          I also completely agree, this needs to go back to 2.5 so that 2.5.1 benefits from these changes as well.
          This ensures that there are less upgrade issues in the future as the old bootstrapbase/clean theme layouts will have only existed for a single minor version.

          Up for integration now.
          Integrator: Andrew N is going to have a look at the JS tomorrow if he has time, feel free to hold off and have a chat to him about the JS side of things.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Damyon, I've added two additional commits, the first to fix the RTL dnd issue Mary linked to that was still present in my changes (easy fix) and the second to add details to theme/upgrade.txt. I also completely agree, this needs to go back to 2.5 so that 2.5.1 benefits from these changes as well. This ensures that there are less upgrade issues in the future as the old bootstrapbase/clean theme layouts will have only existed for a single minor version. Up for integration now. Integrator : Andrew N is going to have a look at the JS tomorrow if he has time, feel free to hold off and have a chat to him about the JS side of things. Many thanks Sam
          Hide
          Gareth J Barnard added a comment - - edited

          Hi Sam Hemelryk,

          Will there be a potential conflict with my proposed fix for MDL-40065 and does your code already cope with the issue?

          Kind regards,

          Gareth

          P.S. GitHub cannot find https://github.com/samhemelryk/moodle/compare/MOODLE_25_STABLE...wip-MDL-39824-m25

          Show
          Gareth J Barnard added a comment - - edited Hi Sam Hemelryk , Will there be a potential conflict with my proposed fix for MDL-40065 and does your code already cope with the issue? Kind regards, Gareth P.S. GitHub cannot find https://github.com/samhemelryk/moodle/compare/MOODLE_25_STABLE...wip-MDL-39824-m25
          Hide
          Mary Evans added a comment - - edited

          Gareth, I think that is because the fix is actually only for 2.6 so will only be in master. At least that is what I am lead to believe.

          PS: Sorry...take that back...did not see Sam's post I only saw yours.

          Show
          Mary Evans added a comment - - edited Gareth, I think that is because the fix is actually only for 2.6 so will only be in master. At least that is what I am lead to believe. PS: Sorry...take that back...did not see Sam's post I only saw yours.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Sam Hemelryk,

          How will 'columns2.php' cope if:

          'regions' => array('side-pre'),
          

          becomes

          'regions' => array('side-post'),
          

          as therefore:

          if (!right_to_left()) {
              echo $OUTPUT->blocks('side-pre', 'span3 desktop-first-column');
          }
          

          would fail?

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Sam Hemelryk , How will 'columns2.php' cope if: 'regions' => array('side-pre'), becomes 'regions' => array('side-post'), as therefore: if (!right_to_left()) { echo $OUTPUT->blocks('side-pre', 'span3 desktop-first-column'); } would fail? Thanks, Gareth
          Hide
          Mary Evans added a comment -

          Even so...as 2.5 is in sync with 2.6 should be easy for them that now how to do this...backporting stuff.

          Show
          Mary Evans added a comment - Even so...as 2.5 is in sync with 2.6 should be easy for them that now how to do this...backporting stuff.
          Show
          Mary Evans added a comment - Have you seen this commit? https://github.com/samhemelryk/moodle/commit/36b77e3a88ef9c5de8422a15e24821a35828048f
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          I did as solves LTR and RTL swapping but I think not when using LTR and post only.

          Unless missing something.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , I did as solves LTR and RTL swapping but I think not when using LTR and post only. Unless missing something. Cheers, Gareth
          Hide
          Mary Evans added a comment -

          I'm just about to test it all again.

          Show
          Mary Evans added a comment - I'm just about to test it all again.
          Hide
          Gareth J Barnard added a comment -

          Sorry Mary Evans - I can check tomorrow too. I just can't see how the blocks method copes (and subsequent calls within) if the region does not exist and no guard code in the layout file.

          Show
          Gareth J Barnard added a comment - Sorry Mary Evans - I can check tomorrow too. I just can't see how the blocks method copes (and subsequent calls within) if the region does not exist and no guard code in the layout file.
          Hide
          Sam Hemelryk added a comment -

          Aha thanks for the heads up about the 2.5 branch not being on github. I've fixed that now.

          Gareth I'd bet this will affect your fix for MDL-40065, hopefully it will resolve that issue, I commented there a little earlier and I see you've replied thanks
          You asked about changing side-pre to side-post. With the changes I have here rtl users get their regions automatically switched based upon a new value you can set in your theme's config.php file. Have a look here: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39824-m26#L13R176
          There is a bit of a gotcha with this, if you look at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39824-m26#L15R75 and line 81 you'll see that columns2.php actually uses two regions side-pre and side-post despite only really having a single block region. Because the switch is defined we must ensure that side-pre and side-post both exist there they'll still be switched even though there is only a single block region.
          I've not thought up a good solution to this yet, but hopefully there is one out there.

          Hope that helps
          Sam

          Show
          Sam Hemelryk added a comment - Aha thanks for the heads up about the 2.5 branch not being on github. I've fixed that now. Gareth I'd bet this will affect your fix for MDL-40065 , hopefully it will resolve that issue, I commented there a little earlier and I see you've replied thanks You asked about changing side-pre to side-post. With the changes I have here rtl users get their regions automatically switched based upon a new value you can set in your theme's config.php file. Have a look here: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39824-m26#L13R176 There is a bit of a gotcha with this, if you look at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39824-m26#L15R75 and line 81 you'll see that columns2.php actually uses two regions side-pre and side-post despite only really having a single block region. Because the switch is defined we must ensure that side-pre and side-post both exist there they'll still be switched even though there is only a single block region. I've not thought up a good solution to this yet, but hopefully there is one out there. Hope that helps Sam
          Hide
          Gareth J Barnard added a comment -

          Sorry but looking at:

          <div id="page-content" class="row-fluid">
                  <div id="region-bs-main-and-pre" class="span9">
                      <div class="row-fluid">
                          <section id="region-main" class="span9 pull-right">
                              <?php
                              echo $OUTPUT->course_content_header();
                              echo $OUTPUT->main_content();
                              echo $OUTPUT->course_content_footer();
                              ?>
                          </section>
                          <?php
                          if (!right_to_left()) {
                              echo $OUTPUT->blocks('side-pre', 'span3 desktop-first-column');
                          } ?>
                      </div>
                  </div>
                  <?php
                  if (right_to_left()) {
                      echo $OUTPUT->blocks('side-post', 'span3');
                  }
                  ?>
              </div>
          

          In 'columns2.php' and 'Fluid grids utilize nesting differently: each nested level of columns should add up to 12 columns' under 'Fluid nesting' on 'http://twitter.github.io/bootstrap/scaffolding.html#fluidGridSystem' there is a logic flaw I think in the code where nested 'row-fluid's do not add up to '12' columns. Outer nest for LTR and inner nest for RTL.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Sorry but looking at: <div id= "page-content" class= "row-fluid" > <div id= "region-bs-main-and-pre" class= "span9" > <div class= "row-fluid" > <section id= "region-main" class= "span9 pull-right" > <?php echo $OUTPUT->course_content_header(); echo $OUTPUT->main_content(); echo $OUTPUT->course_content_footer(); ?> </section> <?php if (!right_to_left()) { echo $OUTPUT->blocks('side-pre', 'span3 desktop-first-column'); } ?> </div> </div> <?php if (right_to_left()) { echo $OUTPUT->blocks('side-post', 'span3'); } ?> </div> In 'columns2.php' and 'Fluid grids utilize nesting differently: each nested level of columns should add up to 12 columns' under 'Fluid nesting' on 'http://twitter.github.io/bootstrap/scaffolding.html#fluidGridSystem' there is a logic flaw I think in the code where nested 'row-fluid's do not add up to '12' columns. Outer nest for LTR and inner nest for RTL. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Thanks for the information Sam Hemelryk - I'll have a look and see what I can do to help with a solution.

          Show
          Gareth J Barnard added a comment - Thanks for the information Sam Hemelryk - I'll have a look and see what I can do to help with a solution.
          Hide
          Sam Hemelryk added a comment -

          Aha, yes the grid system changes. Again a downside to this whole change set when looking a the two columns layout and switching block positions from left to right.
          I worked around this here: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39824-m26#L21R28
          That uses the bootstrap mixin used in defining the grids to adjust the column layout in the circumstance side-post is being used there.

          Show
          Sam Hemelryk added a comment - Aha, yes the grid system changes. Again a downside to this whole change set when looking a the two columns layout and switching block positions from left to right. I worked around this here: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-39824-m26#L21R28 That uses the bootstrap mixin used in defining the grids to adjust the column layout in the circumstance side-post is being used there.
          Hide
          Gareth J Barnard added a comment -

          Dear Sam Hemelryk,

          Thanks I see. FYI in case you want to remove it and use something else (or difficult for theme designers perhaps), I came up with a bit of logic for this to add / remove 'pull-right' and 'desktop-first-column' in LTR / RTL flip flop in https://github.com/gjb2048/moodle-theme_shoelace/blob/228074cf1837e1aab635d6e44ed8854272b3c81d/layout/general.php

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Sam Hemelryk , Thanks I see. FYI in case you want to remove it and use something else (or difficult for theme designers perhaps), I came up with a bit of logic for this to add / remove 'pull-right' and 'desktop-first-column' in LTR / RTL flip flop in https://github.com/gjb2048/moodle-theme_shoelace/blob/228074cf1837e1aab635d6e44ed8854272b3c81d/layout/general.php Cheers, Gareth
          Hide
          Mary Evans added a comment -

          They do add up to 12 in both column3.php and column2.php

          9 + 3 or 3 + 9 in (column2.php)
          and (8+4 nested inside 9) + 3 in column3.php

          Show
          Mary Evans added a comment - They do add up to 12 in both column3.php and column2.php 9 + 3 or 3 + 9 in (column2.php) and (8+4 nested inside 9) + 3 in column3.php
          Hide
          Gareth J Barnard added a comment -

          P.S. If in the future I could come up with a simple change to 'columns2.php' to cope with this and remove 'moodle/core.less' grid changes would you accept the patch?

          Show
          Gareth J Barnard added a comment - P.S. If in the future I could come up with a simple change to 'columns2.php' to cope with this and remove 'moodle/core.less' grid changes would you accept the patch?
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          In 'column3.php' they do add up to 12 in both nests but in 'column2.php' they don't because of the guard 'if (!right_to_left())' etc.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , In 'column3.php' they do add up to 12 in both nests but in 'column2.php' they don't because of the guard 'if (!right_to_left())' etc. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Sam Hemelryk,

          Just to ask, you said for 'columns2.php' -> 'Because the switch is defined we must ensure that side-pre and side-post both exist there they'll still be switched even though there is only a single block region.' then as the switching code for RTL kicks in ('blocks' method) to transform 'side-post' to 'side-pre' (as 'side-post' is used for RTL and 'side-pre' for LTR with no switch code - initiated in the 'columns2.php' file) before 'blocks_for_region' is called then therefore only 'side-pre' has to exist?

          Therefore we just need to detect if 'side-pre' or 'side-post' is being used in 'config.php' and then use the appropriate 'flip-flop' parameter value to the 'blocks' method call in 'columns2.php'.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Sam Hemelryk , Just to ask, you said for 'columns2.php' -> 'Because the switch is defined we must ensure that side-pre and side-post both exist there they'll still be switched even though there is only a single block region.' then as the switching code for RTL kicks in ('blocks' method) to transform 'side-post' to 'side-pre' (as 'side-post' is used for RTL and 'side-pre' for LTR with no switch code - initiated in the 'columns2.php' file) before 'blocks_for_region' is called then therefore only 'side-pre' has to exist? Therefore we just need to detect if 'side-pre' or 'side-post' is being used in 'config.php' and then use the appropriate 'flip-flop' parameter value to the 'blocks' method call in 'columns2.php'. Cheers, Gareth
          Hide
          Mary Evans added a comment - - edited

          Gareth, depending on whether the blocks are side-pre-only or side-post-only in the 2 column page, when in RTL lang mode whatever blocks are normally loaded in side-pre (blocks for region-pre) these are replaced by (block for region-post). The column does not swap only the contents. Same goes for side-post-only, what would normally be (blocks for region-post) become (blocks for region-pre).

          So the condition needs to included directly after
          if it's NOT RTL then load blocks for side-pre
          other wise load blocks for side post

          Show
          Mary Evans added a comment - - edited Gareth, depending on whether the blocks are side-pre-only or side-post-only in the 2 column page, when in RTL lang mode whatever blocks are normally loaded in side-pre (blocks for region-pre) these are replaced by (block for region-post). The column does not swap only the contents. Same goes for side-post-only, what would normally be (blocks for region-post) become (blocks for region-pre). So the condition needs to included directly after if it's NOT RTL then load blocks for side-pre other wise load blocks for side post
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          Well - this is really rather major change for 2.5, but it seems to have widespread support and I take the argument that this will lead to better long term gains.

          My only comment is that I think that both branches should mention the theme changes in the upgrade.txt as 2.5.1, rather than 2.6 - what do you think?

          Show
          Dan Poltawski added a comment - Hi Sam, Well - this is really rather major change for 2.5, but it seems to have widespread support and I take the argument that this will lead to better long term gains. My only comment is that I think that both branches should mention the theme changes in the upgrade.txt as 2.5.1, rather than 2.6 - what do you think?
          Hide
          Sam Hemelryk added a comment -

          Yip now that I think about it I agree Dan, I've pushed up amended branches changing 2.6 to 2.5.1 in upgrade.txt (separate commit).

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Yip now that I think about it I agree Dan, I've pushed up amended branches changing 2.6 to 2.5.1 in upgrade.txt (separate commit). Many thanks Sam
          Hide
          Dan Poltawski added a comment -

          Integrated to master and 2.6, thanks Sam.

          Let the testing begin!

          Show
          Dan Poltawski added a comment - Integrated to master and 2.6, thanks Sam. Let the testing begin!
          Hide
          David Monllaó added a comment -

          As proposed we can create a shared test session to ensure there are no regressions

          Show
          David Monllaó added a comment - As proposed we can create a shared test session to ensure there are no regressions
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Sam Hemelryk,

          I've done a pull request on your m26 branch of the code with a simplification of the 'columns2.php' file as you expressed an interest in 'I've not thought up a good solution to this yet, but hopefully there is one out there.' as it:

          Eliminates the nesting.
          Removes the extra grid system changes.
          Works for LTR and RTL.
          Copes with 'side-post' and no 'side-pre'.

          I've removed the grid altering code in 'moodle/core.less'.

          'columns2.php' snippet:

          $ltr = (!right_to_left());  // Also used to know if to add 'pull-right' and 'desktop-first-column' classes in the layout for LTR.
          $pre = 'side-pre';
          $post = 'side-post';
          if (!$ltr) { // In RTL the sides are reversed.
              // Swap....
              $temp = $pre;
              $pre = $post;
              $post = $temp;
          }
          $hassidepre = (empty($PAGE->layout_options['noblocks']) && $PAGE->blocks->region_has_content('side-pre', $OUTPUT));
          /* This copes with the value of 'regions' being 'side-pre' or 'side-post' in 'config.php'.
             If there is a 'side-pre' then use it, the RTL logic above means that 'side-post' will be in $useblock but then this
             will be converted back to 'side-pre' by the swapping code in the method 'block' as it uses the '$THEME->blockrtlmanipulations'
             array in 'config.php'.  If there is not a 'side-pre' and a 'side-post' has not been defined then this is a developers coding
             fault in 'config.php' and therefore would need to be reported.
          */
          if ($hassidepre) {
              $useblock = $pre;
          } else {
              $useblock = $post;
          }
          ...
              <div id="page-content" class="row-fluid">
                  <section id="region-main" class="span9<?php if ($ltr) { echo ' pull-right'; } ?>">
                      <?php
                      echo $OUTPUT->course_content_header();
                      echo $OUTPUT->main_content();
                      echo $OUTPUT->course_content_footer();
                      ?>
                  </section>
                  <?php
                  $classextra = '';
                  if ($ltr) {
                      $classextra = ' desktop-first-column';
                  }
                  echo $OUTPUT->blocks($useblock, 'span3'.$classextra);
                  ?>
              </div>
          

          If it's too late and as has been said in the past 'the bearded man has sung' - then I can put into 'MDL-40065'.

          And Mary Evans - I think this solves the RTL question you were asking of me

          If anybody wants to test my branch it's located at: https://github.com/gjb2048/moodle/tree/wip-MDL-39824-m26 / diff on https://github.com/gjb2048/moodle/commit/3962de9d52e425cb5f076ef0bf25cd80dff35f24

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Sam Hemelryk , I've done a pull request on your m26 branch of the code with a simplification of the 'columns2.php' file as you expressed an interest in 'I've not thought up a good solution to this yet, but hopefully there is one out there.' as it: Eliminates the nesting. Removes the extra grid system changes. Works for LTR and RTL. Copes with 'side-post' and no 'side-pre'. I've removed the grid altering code in 'moodle/core.less'. 'columns2.php' snippet: $ltr = (!right_to_left()); // Also used to know if to add 'pull-right' and 'desktop-first-column' classes in the layout for LTR. $pre = 'side-pre'; $post = 'side-post'; if (!$ltr) { // In RTL the sides are reversed. // Swap.... $temp = $pre; $pre = $post; $post = $temp; } $hassidepre = (empty($PAGE->layout_options['noblocks']) && $PAGE->blocks->region_has_content('side-pre', $OUTPUT)); /* This copes with the value of 'regions' being 'side-pre' or 'side-post' in 'config.php'. If there is a 'side-pre' then use it, the RTL logic above means that 'side-post' will be in $useblock but then this will be converted back to 'side-pre' by the swapping code in the method 'block' as it uses the '$THEME->blockrtlmanipulations' array in 'config.php'. If there is not a 'side-pre' and a 'side-post' has not been defined then this is a developers coding fault in 'config.php' and therefore would need to be reported. */ if ($hassidepre) { $useblock = $pre; } else { $useblock = $post; } ... <div id= "page-content" class= "row-fluid" > <section id= "region-main" class= "span9<?php if ($ltr) { echo ' pull-right'; } ?>" > <?php echo $OUTPUT->course_content_header(); echo $OUTPUT->main_content(); echo $OUTPUT->course_content_footer(); ?> </section> <?php $classextra = ''; if ($ltr) { $classextra = ' desktop-first-column'; } echo $OUTPUT->blocks($useblock, 'span3'.$classextra); ?> </div> If it's too late and as has been said in the past 'the bearded man has sung' - then I can put into ' MDL-40065 '. And Mary Evans - I think this solves the RTL question you were asking of me If anybody wants to test my branch it's located at: https://github.com/gjb2048/moodle/tree/wip-MDL-39824-m26 / diff on https://github.com/gjb2048/moodle/commit/3962de9d52e425cb5f076ef0bf25cd80dff35f24 Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Now simplified the one column layout to remove an extra 'div' -> https://github.com/gjb2048/moodle/commit/9285430d3e584e12768aa65b5ef0f4b49f546496 which gives a layout of:

              <div id="page-content" class="row-fluid">
                  <section id="region-main" class="span12">
                      <?php
                      echo $OUTPUT->course_content_header();
                      echo $OUTPUT->main_content();
                      echo $OUTPUT->course_content_footer();
                      ?>
                  </section>
              </div>
          

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Now simplified the one column layout to remove an extra 'div' -> https://github.com/gjb2048/moodle/commit/9285430d3e584e12768aa65b5ef0f4b49f546496 which gives a layout of: <div id= "page-content" class= "row-fluid" > <section id= "region-main" class= "span12" > <?php echo $OUTPUT->course_content_header(); echo $OUTPUT->main_content(); echo $OUTPUT->course_content_footer(); ?> </section> </div> Cheers, Gareth
          Hide
          Mary Evans added a comment -

          Gareth, if you look at base theme the report.php does not have the swap sides for RTL. But if you look in Afterburner that does not use a report page it uses the 3 col page default.php that allows the blocks for side-post blocks to be loaded into side-pre for RTL swap.

          So basically the thing that is missing in Bootstrap themes is the $hassidepost declaration otherwise there is no point in having the RTL swap code in that 2 col page.

          Also the logic which is swaping using the temp isn't really necessary as you can do this with if else, but if you don't have any side-post blocks to load, there is no point in any of this logic as it's not going to do anything other generate an error looking for side-post surely?

          Show
          Mary Evans added a comment - Gareth, if you look at base theme the report.php does not have the swap sides for RTL. But if you look in Afterburner that does not use a report page it uses the 3 col page default.php that allows the blocks for side-post blocks to be loaded into side-pre for RTL swap. So basically the thing that is missing in Bootstrap themes is the $hassidepost declaration otherwise there is no point in having the RTL swap code in that 2 col page. Also the logic which is swaping using the temp isn't really necessary as you can do this with if else, but if you don't have any side-post blocks to load, there is no point in any of this logic as it's not going to do anything other generate an error looking for side-post surely?
          Hide
          Mary Evans added a comment -

          Forgot there is a rtlmanipulation setting...so forget last outburst! LOL

          Show
          Mary Evans added a comment - Forgot there is a rtlmanipulation setting...so forget last outburst! LOL
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          No problem - its a bit tricky to grasp hence the comment. I can't comment on how 'columns2.php' is used or what is suitable for RTL - perhaps other themes are wrong?

          Current logic in the fix does not need '$hassidepost' as only two states. So if '$hassidepre' is false then '$hassidepost' must be true for 'column2.php' or else coding fault in 'config.php'.

          Tested:
          'side-pre' and LTR
          'side-post' and LTR
          'side-pre' and RTL
          'side-post' and RTL

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , No problem - its a bit tricky to grasp hence the comment. I can't comment on how 'columns2.php' is used or what is suitable for RTL - perhaps other themes are wrong? Current logic in the fix does not need '$hassidepost' as only two states. So if '$hassidepre' is false then '$hassidepost' must be true for 'column2.php' or else coding fault in 'config.php'. Tested: 'side-pre' and LTR 'side-post' and LTR 'side-pre' and RTL 'side-post' and RTL Cheers, Gareth
          Hide
          Damyon Wiese added a comment -

          Hi Gareth,

          This issue has already been integrated - your change sounds like a further improvement, so please split it into a separate issue (MDL-40065 sounds fine).

          Thanks!

          Show
          Damyon Wiese added a comment - Hi Gareth, This issue has already been integrated - your change sounds like a further improvement, so please split it into a separate issue ( MDL-40065 sounds fine). Thanks!
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          No problem and will do.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , No problem and will do. Cheers, Gareth
          Hide
          Mary Evans added a comment - - edited

          @Gareth, there are not themes in Moodle core that use the RTL block swap other than Afterburner, and Base (as it is Parent theme).

          Afterburner uses a three column page for all layouts, but takes its default region declared in afterburner/config.php. Report and Admin layouts use Side Pre only. This theme works correctly and has had not had any problems to date with RTL issues.

          Show
          Mary Evans added a comment - - edited @Gareth, there are not themes in Moodle core that use the RTL block swap other than Afterburner, and Base (as it is Parent theme). Afterburner uses a three column page for all layouts, but takes its default region declared in afterburner/config.php. Report and Admin layouts use Side Pre only. This theme works correctly and has had not had any problems to date with RTL issues.
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          No problem, it was a question and did not have time to look as NetBook power cable was playing up and was concerned about that .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , No problem, it was a question and did not have time to look as NetBook power cable was playing up and was concerned about that . Cheers, Gareth
          Hide
          David Monllaó added a comment -

          No regressions detected, but waiting for behat results (tomorrow morning) to pass it.

          Show
          David Monllaó added a comment - No regressions detected, but waiting for behat results (tomorrow morning) to pass it.
          Hide
          Dan Poltawski added a comment - - edited

          I'm seeing this when going to the course index page ( /course/index.php)

          ( ! ) Notice: Undefined variable: breadcrumbs in /Users/danp/git/integration/theme/bootstrapbase/renderers/core_renderer.php on line 61
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0007	259656	{main}( )	../index.php:0
          2	0.2420	23217832	core_renderer->header( )	../index.php:59
          3	0.2524	23579552	core_renderer->render_page_layout( )	../outputrenderers.php:777
          4	0.2526	23616032	include( '/Users/danp/git/integration/theme/clean/layout/columns3.php' )	../outputrenderers.php:847
          5	1.5022	54546496	theme_bootstrapbase_core_renderer->navbar( )	../columns3.php:72
          
          ( ! ) Warning: join(): Invalid arguments passed in /Users/danp/git/integration/theme/bootstrapbase/renderers/core_renderer.php on line 61
          Call Stack
          #	Time	Memory	Function	Location
          1	0.0007	259656	{main}( )	../index.php:0
          2	0.2420	23217832	core_renderer->header( )	../index.php:59
          3	0.2524	23579552	core_renderer->render_page_layout( )	../outputrenderers.php:777
          4	0.2526	23616032	include( '/Users/danp/git/integration/theme/clean/layout/columns3.php' )	../outputrenderers.php:847
          5	1.5022	54546496	theme_bootstrapbase_core_renderer->navbar( )	../columns3.php:72
          6	1.5064	54551288	join ( )	../core_renderer.php:61
          Page path
          
          
          Show
          Dan Poltawski added a comment - - edited I'm seeing this when going to the course index page ( /course/index.php) ( ! ) Notice: Undefined variable: breadcrumbs in /Users/danp/git/integration/theme/bootstrapbase/renderers/core_renderer.php on line 61 Call Stack # Time Memory Function Location 1 0.0007 259656 {main}( ) ../index.php:0 2 0.2420 23217832 core_renderer->header( ) ../index.php:59 3 0.2524 23579552 core_renderer->render_page_layout( ) ../outputrenderers.php:777 4 0.2526 23616032 include( '/Users/danp/git/integration/theme/clean/layout/columns3.php' ) ../outputrenderers.php:847 5 1.5022 54546496 theme_bootstrapbase_core_renderer->navbar( ) ../columns3.php:72 ( ! ) Warning: join(): Invalid arguments passed in /Users/danp/git/integration/theme/bootstrapbase/renderers/core_renderer.php on line 61 Call Stack # Time Memory Function Location 1 0.0007 259656 {main}( ) ../index.php:0 2 0.2420 23217832 core_renderer->header( ) ../index.php:59 3 0.2524 23579552 core_renderer->render_page_layout( ) ../outputrenderers.php:777 4 0.2526 23616032 include( '/Users/danp/git/integration/theme/clean/layout/columns3.php' ) ../outputrenderers.php:847 5 1.5022 54546496 theme_bootstrapbase_core_renderer->navbar( ) ../columns3.php:72 6 1.5064 54551288 join ( ) ../core_renderer.php:61 Page path
          Hide
          Dan Poltawski added a comment -

          I don't think the above issue is related to this change. $this->page->navbar->get_items(); is returning no items on my front page when i'm logged in, I have yet to be able to trace the cause of this.

          Having said that, we could probably make this more robust by initalising the array

          Show
          Dan Poltawski added a comment - I don't think the above issue is related to this change. $this->page->navbar->get_items(); is returning no items on my front page when i'm logged in, I have yet to be able to trace the cause of this. Having said that, we could probably make this more robust by initalising the array
          Hide
          Dan Poltawski added a comment -

          Actually these warnings are being created where they weren't before this patch - see MDL-40174 to see how to reproduce.

          Show
          Dan Poltawski added a comment - Actually these warnings are being created where they weren't before this patch - see MDL-40174 to see how to reproduce.
          Hide
          Dan Poltawski added a comment -

          I'm setting this to failed, as I believe that this change is handling the bug in MDL-40174 less gracefully than before. Even though thats a bug too.

          Show
          Dan Poltawski added a comment - I'm setting this to failed, as I believe that this change is handling the bug in MDL-40174 less gracefully than before. Even though thats a bug too.
          Hide
          Dan Poltawski added a comment -

          I've pushed a fix for the warning, so the rest is handled in the linked issue.

          Show
          Dan Poltawski added a comment - I've pushed a fix for the warning, so the rest is handled in the linked issue.
          Show
          David Monllaó added a comment - The tests finished with errors regarding: https://tracker.moodle.org/browse/MDL-37459?focusedCommentId=228214&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-228214 https://tracker.moodle.org/browse/MDL-37621?focusedCommentId=228212&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-228212 After applying both issues quick fixes behat tests are 100% passing
          Hide
          Marina Glancy added a comment -

          Thanks for your awesome work! This has now become a part of Moodle.

          Closing as fixed!

          Show
          Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!
          Hide
          Gareth J Barnard added a comment -

          Hi all,

          As an after thought, does the Bootstrap 'version.php' file need changing due to the changes in core? And therefore themes that depend on it can change their dependency requirement as they may not work now with these changes?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi all, As an after thought, does the Bootstrap 'version.php' file need changing due to the changes in core? And therefore themes that depend on it can change their dependency requirement as they may not work now with these changes? Cheers, Gareth
          Hide
          Mary Evans added a comment -

          I'm not absolutely sure, but I think all themes get updated together when a new version of Moodle is released. So the next one due is 2.5.1, so I would think that will be the most likely time, if any. But I am know to get things wrong most of the time.

          Show
          Mary Evans added a comment - I'm not absolutely sure, but I think all themes get updated together when a new version of Moodle is released. So the next one due is 2.5.1, so I would think that will be the most likely time, if any. But I am know to get things wrong most of the time.
          Hide
          Mary Cooch added a comment -

          Just housekeeping here -I noticed the docs_required label but don't have the knowledge to document it myself - it would be helpful if a themes expert could add something to http://docs.moodle.org/26/en/Themes or where appropriate; thanks

          Show
          Mary Cooch added a comment - Just housekeeping here -I noticed the docs_required label but don't have the knowledge to document it myself - it would be helpful if a themes expert could add something to http://docs.moodle.org/26/en/Themes or where appropriate; thanks

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: