Moodle
  1. Moodle
  2. MDL-20204

Themes 2.0: YUI, Caching, Performance, Consistency

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: None
    • Component/s: Themes
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      418

      Description

      See http://docs.moodle.org/en/Development:Theme_changes_proposal
      going to post patches here and add subtasks

      Please note this projects was not confirmed yet, we are at the prototype stage that has to be approved by:
      1/ core developers - Tim and MD
      2/ theme designers - Patrick and Manuo

        Issue Links

        Progress
        Resolved Sub-Tasks

        Sub-Tasks

        1.
        remove all theme backwards compatibility hooks Sub-task Closed Petr Škoda
         
        2.
        replace image and icon handling Sub-task Closed Petr Škoda
         
        3.
        change page "general type" to "layout" in themes and other APIs Sub-task Closed Petr Škoda
         
        4.
        new theme config.php internal structure Sub-task Closed Petr Škoda
         
        5.
        rename moodle_core_renderer to just core_renderer Sub-task Closed Petr Škoda
         
        6.
        Decide once and for all how the output API will look to developers Sub-task Closed Petr Škoda
         
        7.
        The icon in the breadcrumb seems to be broken Sub-task Closed Petr Škoda
         
        8.
        Theme specific CSS for plugins: styles_themename.css Sub-task Closed Sam Hemelryk
         
        9.
        Collapse/expand (-/+) block icons missing Sub-task Closed Petr Škoda
         
        10.
        use correct component prefixes in theme related APIs Sub-task Closed Petr Škoda
         
        11.
        remove renderer that tries to emulate templating system Sub-task Closed Petr Škoda
         
        12.
        import YUI 2.8.0r4 Sub-task Closed Petr Škoda
         
        13.
        import YUI 3.0 Sub-task Closed Petr Škoda
         
        14.
        new OUTPUT->single_select() and OUTPUT->single_textbox() Sub-task Closed Petr Škoda
         
        15.
        fix or remove xhtml_container_stack::__destruct Sub-task Closed Petr Škoda
         
        16.
        reimplement $CFG->themedir Sub-task Closed Petr Škoda
         
        17.
        add full page layout needed for scorm, emebedded resource, etc. Sub-task Closed Petr Škoda
         
        18.
        fully replace smartpix Sub-task Closed Petr Škoda
         
        19.
        split langmenu and logininfo into separate divs + let themes decide when/where to print them Sub-task Closed Petr Škoda
         
        20.
        move all plugin CSS into old standard theme and then gradually move it back when improved and compatible with base theme Sub-task Closed Petr Škoda
         
        21.
        implement theme/javascript.php Sub-task Closed Petr Škoda
         
        22.
        remove unnecessary global $THEME Sub-task Closed Petr Škoda
         
        23.
        find a way to let yui2 and yui3 coexist Sub-task Closed Petr Škoda
         
        24.
        switch to CSS reset from YUI 3 Sub-task Closed Petr Škoda
         
        25.
        solve somehow theme CSS and rendering in emails Sub-task Closed Petr Škoda
         
        26.
        fix file picker regression cause most probably by the YUI 2.8.0r4 upgrade Sub-task Closed Dongsheng Cai
         
        27.
        moodle_url improvements Sub-task Closed Petr Škoda
         
        28.
        prevent abusing of renderer factories, the component has to be always valid string Sub-task Closed Petr Škoda
         
        29.
        do not use standard "get_x" methods for "magic" $PAGE stuff Sub-task Closed Petr Škoda
         
        30.
        add renderer target parameter to$PAGE->get_renderer() Sub-task Closed Petr Škoda
         
        31.
        should plugin renderers redirect calls to core renderer methods? Sub-task Closed Petr Škoda
         
        32.
        review $CFG->allowthemechangeonurl implementation Sub-task Closed Petr Škoda
         
        33.
        Use standard PHPloader for YUI2/3 Sub-task Closed Petr Škoda
         
        34.
        remove support for YUI pix folder in theme Sub-task Closed Petr Škoda
         
        35.
        reimplement $OUTPUT->help_scales() Sub-task Closed Petr Škoda
         
        36.
        implement custom yuicombo resource loader Sub-task Closed Petr Škoda
         
        37.
        make yui3 loader (Y.use()) to load from our site by default, not yahoo Sub-task Closed Petr Škoda
         
        38.
        output components can not use globals $PAGE and $OUTPUT Sub-task Closed Petr Škoda
         
        39.
        improve designer mode performance Sub-task Closed Petr Škoda
         
        40.
        add $THEME->javascripts_footer = array() option Sub-task Closed Petr Škoda
         
        41.
        minimise theme CSS - improves browser caching Sub-task Closed Petr Škoda
         
        42.
        set up correct debugging in ABORT_AFTER_CONFIG scripts Sub-task Closed Petr Škoda
         
        43.
        make sure theme caches are invalidated after each upgrade Sub-task Closed Petr Škoda
         
        44.
        move yui2 skin from the huge CSS theme stylesheet Sub-task Closed Petr Škoda
         
        45.
        implement gzip compression in CSS and JS resources in theme/*.php Sub-task Closed Petr Škoda
         
        46.
        action_link always forces a popup... not sure it that is the desired action or not Sub-task Closed Petr Škoda
         
        47.
        fix $OUTPUT autocompletion in Eclipse Sub-task Closed Petr Škoda
         
        48.
        fix sloppy $PAGE->set_url() Sub-task Closed Petr Škoda
         
        49.
        remove all legacy themes from HEAD except standard theme Sub-task Closed Petr Škoda
         
        50.
        implement new theme diagnostic code Sub-task Closed Unassigned
         
        51.
        somehow fix hardcoded page styles in manually created pages and emails Sub-task Closed Unassigned
         
        52.
        redesign navigation sidebar to be compatible with CSS column themes Sub-task Closed Unassigned
         
        53.
        create some new default theme and use it as default theme after install Sub-task Closed Unassigned
         
        54.
        fix special mnet theme tweaks Sub-task Closed David Mudrak
         
        55.
        User profile page description and interests textarea boxes cramped to 12.25em width because of YUI 3? Sub-task Closed moodle.com
         
        56.
        review and improve core output renderer and weblib.php Sub-task Closed Petr Škoda
         
        57.
        define more page layouts Sub-task Closed Petr Škoda
         
        58.
        rewrite theme unit tests Sub-task Closed Unassigned
         
        59.
        improve moodle_url class Sub-task Closed Petr Škoda
         
        60.
        migrate old non-widget YUI2 code to new YUI3 Sub-task Closed Dongsheng Cai
         
        61.
        make sure all core developers understand CSS selectors and use them properly Sub-task Closed Sam Hemelryk
         
        62.
        navbar potential problems Sub-task Closed Unassigned
         
        63.
        add unittests for themes and outputlib Sub-task Closed Petr Škoda
         
        64.
        move html_table processing to html_writer class Sub-task Closed David Mudrak
         
        65.
        review render and outputlib docs Sub-task Closed Petr Škoda
         
        66.
        Completely rewrite installer theming Sub-task Closed Sam Hemelryk
         
        67.
        remove the need to specify the themename from layout description Sub-task Closed Sam Hemelryk
         
        68.
        import YUI3.1.0 Sub-task Closed Petr Škoda
         

          Activity

          Hide
          Petr Škoda added a comment -

          can not attach patch here - it is too big and too many changes in binary files:
          test site at http://ferda.cdv.tul.cz/moodle20t/
          you can downlaod the cvs checkout from http://ferda.cdv.tul.cz/moodle20t.zip

          Missing bits:

          • theme switching page needs improvements - all diagnostics and problems should be showed there, not inline; add button for purging of caches
          • all themes except standard and sample theme removed
          • JS not supported in themes - do we need it?
          • no examples of template use
          • grade edit tree.css need to be fixed and moved into standard theme
          • theme compression
          Show
          Petr Škoda added a comment - can not attach patch here - it is too big and too many changes in binary files: test site at http://ferda.cdv.tul.cz/moodle20t/ you can downlaod the cvs checkout from http://ferda.cdv.tul.cz/moodle20t.zip Missing bits: theme switching page needs improvements - all diagnostics and problems should be showed there, not inline; add button for purging of caches all themes except standard and sample theme removed JS not supported in themes - do we need it? no examples of template use grade edit tree.css need to be fixed and moved into standard theme theme compression
          Hide
          Mauno Korpelainen added a comment -

          Does IE use some custom css - other browsers (FF,Opera,Safari,Chrome) catch the color settings (of samplecolor theme) for css but IE does not... still IE does get the width of left and right colums from settings?

          Show
          Mauno Korpelainen added a comment - Does IE use some custom css - other browsers (FF,Opera,Safari,Chrome) catch the color settings (of samplecolor theme) for css but IE does not... still IE does get the width of left and right colums from settings?
          Hide
          Mauno Korpelainen added a comment -

          I mean that it looks like IE does not read theme/samplecolor/styles at all???

          background-image:url([[themesetting:gradient]]) etc

          Show
          Mauno Korpelainen added a comment - I mean that it looks like IE does not read theme/samplecolor/styles at all??? background-image:url([ [themesetting:gradient] ]) etc
          Hide
          Petr Škoda added a comment -

          oh, I did not test IE yet, going to fix it today!

          Show
          Petr Škoda added a comment - oh, I did not test IE yet, going to fix it today!
          Hide
          Petr Škoda added a comment -

          found the problem, the sloppy IE thing has some weird limit on number of things in CSS - going to chop the large CSS into smaller chunks just for IE

          Show
          Petr Škoda added a comment - found the problem, the sloppy IE thing has some weird limit on number of things in CSS - going to chop the large CSS into smaller chunks just for IE
          Hide
          Mauno Korpelainen added a comment -

          IE can never stop surprising - was it CSS File Size Limit (288kb for IE) , maximum of 32 @import statements or some other limit?

          Show
          Mauno Korpelainen added a comment - IE can never stop surprising - was it CSS File Size Limit (288kb for IE) , maximum of 32 @import statements or some other limit?
          Hide
          Petr Škoda added a comment -

          4096 selectors limit

          Show
          Petr Škoda added a comment - 4096 selectors limit
          Hide
          Mauno Korpelainen added a comment -

          What is the current state of http://docs.moodle.org/en/Development:Theme_changes_proposal Petr?

          Is it already approved and going to be implemented to moodle 2.0 HEAD ?

          Show
          Mauno Korpelainen added a comment - What is the current state of http://docs.moodle.org/en/Development:Theme_changes_proposal Petr? Is it already approved and going to be implemented to moodle 2.0 HEAD ?
          Hide
          Petr Škoda added a comment -

          latest test site at http://ferda.cdv.tul.cz/moodle20t/
          complete cvs checkout at http://ferda.cdv.tul.cz/moodle20t.zip

          unfortunately normal patch has problems with binary files

          Show
          Petr Škoda added a comment - latest test site at http://ferda.cdv.tul.cz/moodle20t/ complete cvs checkout at http://ferda.cdv.tul.cz/moodle20t.zip unfortunately normal patch has problems with binary files
          Hide
          Petr Škoda added a comment -

          adding tim as watcher here, if you find some time please have a loot at the list of subtasks, thanks

          Show
          Petr Škoda added a comment - adding tim as watcher here, if you find some time please have a loot at the list of subtasks, thanks
          Hide
          Martin Dougiamas added a comment -

          Go Petr go! +100

          Show
          Martin Dougiamas added a comment - Go Petr go! +100
          Hide
          Tim Hunt added a comment -

          I'll try to look at this today, if you can hold off a bit after Martin's enthusiasm

          Show
          Tim Hunt added a comment - I'll try to look at this today, if you can hold off a bit after Martin's enthusiasm
          Hide
          Petr Škoda added a comment -

          I am sorry Tim, I can not wait much longer, we have to move forward. I am going to start committing changes later today.

          Show
          Petr Škoda added a comment - I am sorry Tim, I can not wait much longer, we have to move forward. I am going to start committing changes later today.
          Hide
          Tim Hunt added a comment -

          Yes. Sorry Petr. I was working on my own code yesterday. Can you give me until lunchtime today (1:00 your time?)

          Show
          Tim Hunt added a comment - Yes. Sorry Petr. I was working on my own code yesterday. Can you give me until lunchtime today (1:00 your time?)
          Hide
          Tim Hunt added a comment -

          OK, I have now commented on all the subtasks (that I feel the need to comment on). Basically I think the plan is great, apart from a few minor questions/concerns I raise.

          Now I want to look at the code, but I am running out of time on my self-imposed deadline. Please could I have a little bit longer. I promise not to work on anything else until I have finished. (However, I will have a break for lunch )

          Show
          Tim Hunt added a comment - OK, I have now commented on all the subtasks (that I feel the need to comment on). Basically I think the plan is great, apart from a few minor questions/concerns I raise. Now I want to look at the code, but I am running out of time on my self-imposed deadline. Please could I have a little bit longer. I promise not to work on anything else until I have finished. (However, I will have a break for lunch )
          Hide
          Tim Hunt added a comment -

          I have to say, this would be much easier to review if the changes were in git.

          I'm still struggling to get Eclipse to show me the diffs. It keeps asking me for your password, even though I have changed the sharing info after downloading your zip.

          Also, CVS thinks that every file has been modified, even though there are no changes in the file. I guess that is a timestamp thing.

          Show
          Tim Hunt added a comment - I have to say, this would be much easier to review if the changes were in git. I'm still struggling to get Eclipse to show me the diffs. It keeps asking me for your password, even though I have changed the sharing info after downloading your zip. Also, CVS thinks that every file has been modified, even though there are no changes in the file. I guess that is a timestamp thing.
          Hide
          Tim Hunt added a comment -

          Right. CVS finally beaten into submission. I am now in a position to start reviewing code. Please give me a few hours.

          Show
          Tim Hunt added a comment - Right. CVS finally beaten into submission. I am now in a position to start reviewing code. Please give me a few hours.
          Hide
          Tim Hunt added a comment -

          I think 3. is the most important of these points:

          1. admin/appearance.php. I can't work out, do the settings for all themes appear on one page, or a separate page for each theme?

          2. backup/restorelib.php has commented out code. Why?
          Same code commented out, rather than deleted, in other places too. Why?

          3. I note you are removing all separate plugin styles.php files. That is one approach. The other option would be to move as many styles as possible into styles.php files in each plugin. That might be easier to maintain, and would reduce the difference between contrib and standard plugins.

          4. course/lib.php, and other places, are still calling $OUTPUT->old_icon_url. Why?

          5. coures/view.php. Aren't you going to rename $PAGE->set_generaltype('course'); to $PAGE->set_layouttype?

          6. You should create a subtask for
          //TODO: find some better way to disable blocks and minimise footer - pagetype just for this does not seem like a good solution
          //$PAGE->set_generaltype('maxcontent');
          in mod/imscp/view.php

          7. login/index.php does $PAGE->set_generaltype('form'); Surely that should be login?

          8. mod/resource/pix/icon.gif seems to be missing.

          OK, I have now reviewed all the trivial changes (The ones outside lib and theme dirs). I have to go to an orchestra rehearsal now. I will review the rest later.

          Show
          Tim Hunt added a comment - I think 3. is the most important of these points: 1. admin/appearance.php. I can't work out, do the settings for all themes appear on one page, or a separate page for each theme? 2. backup/restorelib.php has commented out code. Why? Same code commented out, rather than deleted, in other places too. Why? 3. I note you are removing all separate plugin styles.php files. That is one approach. The other option would be to move as many styles as possible into styles.php files in each plugin. That might be easier to maintain, and would reduce the difference between contrib and standard plugins. 4. course/lib.php, and other places, are still calling $OUTPUT->old_icon_url. Why? 5. coures/view.php. Aren't you going to rename $PAGE->set_generaltype('course'); to $PAGE->set_layouttype? 6. You should create a subtask for //TODO: find some better way to disable blocks and minimise footer - pagetype just for this does not seem like a good solution //$PAGE->set_generaltype('maxcontent'); in mod/imscp/view.php 7. login/index.php does $PAGE->set_generaltype('form'); Surely that should be login? 8. mod/resource/pix/icon.gif seems to be missing. OK, I have now reviewed all the trivial changes (The ones outside lib and theme dirs). I have to go to an orchestra rehearsal now. I will review the rest later.
          Hide
          Tim Hunt added a comment -

          More.

          9. I do not see any logic to the division

          $THEME->sheets = array(
              'pagelayout',
              'core',
              'blocks',
              'course',
              'block_calendar_month',
          );
          

          10. I do no understand
          'options' => array('nofooter', 'nonavbar'),
          in $THEME->layouts. Surely you can control this in layout.php.

          Doh! No. I see. It lets you reuse general.php in more cases.

          11. Does $THEME need to be a global? Surely we can get rid of it and just use $PAGE->theme to access it. In which case you can delete $THEME-> everywhere in config.php.

          12. This bit of code:

          $regionsinfo = 'pagelayout';
          if ($PAGE->blocks->region_has_content('side-pre', $OUTPUT)) {
              $regionsinfo .= '-pre';
          }
          if ($PAGE->blocks->region_has_content('side-post', $OUTPUT)) {
              $regionsinfo .= '-post';
          }
          

          must not be in the layout PHP file. Instead, make a $PAGE->pagedivclasses like $PAGE->bodyclasses to encapsulate that logic. Actually, how come you need that code in base, but not in standard? I don't understand.

          13. I don't really like the logic

          if (empty($PAGE->layout_options['nonavbar']) and $OUTPUT->has_navbar()) {
          

          for two reasons. First, we should not have logic in layout PHP. so at most it should be

          if (empty($PAGE->has_navbar()) {
          

          Second. $OUTPUT is a renderer. Its sole job is to produce HTML. There is no way that a $OUTPUT->has_navbar() method should exist.

          14. Do we really need that many divs in general.php?

          15. Oh yuck! Just seen $THEME->sheets in theme/standard/config.php. That really convinces me that I am right in 3. above. We should have a styles.css in each plugin.

          16. And I really do not see any point in having both base and standard themes. We should just have a base theme, whatever it is called.

          17. what replaces the old rtl.css?

          18. why is favicon.ico at the top level, not in pix?

          19. I am very doubtful about deleting customcorners and anomaly from CVS. We want them (working) in 2.0. If you delete them now and add them back later, it will make CVS history even harder to follow. I think it is better to leave them there, but broken, until they are fixed.

          20. Please commit the template renderer code somewhere in contrib, so it is easily browsable, rather than deleting it completely.

          21. That is weird, the unit tests have been changed to say set_pagelayout, even though other code still refers to generaltype.

          22. Why is the change in unction path_inaccessdata($path, $accessdata) { in this patch?

          23. lib/javascript-static.js has tabs, not spaces.

          Saving these before my browser crashes. More to come.

          Show
          Tim Hunt added a comment - More. 9. I do not see any logic to the division $THEME->sheets = array( 'pagelayout', 'core', 'blocks', 'course', 'block_calendar_month', ); 10. I do no understand 'options' => array('nofooter', 'nonavbar'), in $THEME->layouts. Surely you can control this in layout.php. Doh! No. I see. It lets you reuse general.php in more cases. 11. Does $THEME need to be a global? Surely we can get rid of it and just use $PAGE->theme to access it. In which case you can delete $THEME-> everywhere in config.php. 12. This bit of code: $regionsinfo = 'pagelayout'; if ($PAGE->blocks->region_has_content('side-pre', $OUTPUT)) { $regionsinfo .= '-pre'; } if ($PAGE->blocks->region_has_content('side-post', $OUTPUT)) { $regionsinfo .= '-post'; } must not be in the layout PHP file. Instead, make a $PAGE->pagedivclasses like $PAGE->bodyclasses to encapsulate that logic. Actually, how come you need that code in base, but not in standard? I don't understand. 13. I don't really like the logic if (empty($PAGE->layout_options['nonavbar']) and $OUTPUT->has_navbar()) { for two reasons. First, we should not have logic in layout PHP. so at most it should be if (empty($PAGE->has_navbar()) { Second. $OUTPUT is a renderer. Its sole job is to produce HTML. There is no way that a $OUTPUT->has_navbar() method should exist. 14. Do we really need that many divs in general.php? 15. Oh yuck! Just seen $THEME->sheets in theme/standard/config.php. That really convinces me that I am right in 3. above. We should have a styles.css in each plugin. 16. And I really do not see any point in having both base and standard themes. We should just have a base theme, whatever it is called. 17. what replaces the old rtl.css? 18. why is favicon.ico at the top level, not in pix? 19. I am very doubtful about deleting customcorners and anomaly from CVS. We want them (working) in 2.0. If you delete them now and add them back later, it will make CVS history even harder to follow. I think it is better to leave them there, but broken, until they are fixed. 20. Please commit the template renderer code somewhere in contrib, so it is easily browsable, rather than deleting it completely. 21. That is weird, the unit tests have been changed to say set_pagelayout, even though other code still refers to generaltype. 22. Why is the change in unction path_inaccessdata($path, $accessdata) { in this patch? 23. lib/javascript-static.js has tabs, not spaces. Saving these before my browser crashes. More to come.
          Hide
          Tim Hunt added a comment -

          11. Correction. Of coures, we don't want to delete $THEME->, because it is an instance of theme_config, which gives you autocomplete and PHPdoc comments. But if it is no longer a global, we could change it to $theme. Or not bother.

          24. I don't think the name minlib.php really explains what it is. I can't currently think of a better name though.

          25. //TODO: add themedir support in moodlelib.php should have a subtask filed.

          26. Coding guidelines say two blank lines between classes. http://docs.moodle.org/en/Development:Coding_style#Class_declarations - you have removed at least on of these in outputfactories.php

          27. I don't understand why you want theme renderers to be in lib.php, while everywhere else they live in renderers.php. Is there a reason? Oh it is slightly more complicated than that. It is renderers.php compared to renderer.php. Hmm. theme_config constructor still mentions renderes.php as well as lib.php. Now I am really confused.

          28. The theme_config properties that are not settable from config.php. Wouldn't it be better to make them protected, with getters, to avoid the possibility of abuse.

          29. You have tabs in outputlib.php.

          30. Theme diagnose($themename) function is a great idea! However, please create a subtask for the TODO.

          31. theme_config constructor should be called __construct.

          32. css_content method is very long, and looks to have a lot of repetitious code. Can't you refactor it?

          33. Was it really right to delete the two existing css post-processing funcitions?

          34. resolve_image_location also looks like it should be refactored to avoid duplication.

          35. In lib/outputrenderers.php, line 327, I think you should use $this->output_empty_tag, to keep the coding style consistent.

          36. Please commit the whitespace fixes in webdavlib.php as a separate commit.

          I hope you don't feel too bad about the number of things I found to comment on. It is mostly excellent, but I think these points need to be addressed before it is ready for CVS.

          Show
          Tim Hunt added a comment - 11. Correction. Of coures, we don't want to delete $THEME->, because it is an instance of theme_config, which gives you autocomplete and PHPdoc comments. But if it is no longer a global, we could change it to $theme. Or not bother. 24. I don't think the name minlib.php really explains what it is. I can't currently think of a better name though. 25. //TODO: add themedir support in moodlelib.php should have a subtask filed. 26. Coding guidelines say two blank lines between classes. http://docs.moodle.org/en/Development:Coding_style#Class_declarations - you have removed at least on of these in outputfactories.php 27. I don't understand why you want theme renderers to be in lib.php, while everywhere else they live in renderers.php. Is there a reason? Oh it is slightly more complicated than that. It is renderers.php compared to renderer.php. Hmm. theme_config constructor still mentions renderes.php as well as lib.php. Now I am really confused. 28. The theme_config properties that are not settable from config.php. Wouldn't it be better to make them protected, with getters, to avoid the possibility of abuse. 29. You have tabs in outputlib.php. 30. Theme diagnose($themename) function is a great idea! However, please create a subtask for the TODO. 31. theme_config constructor should be called __construct. 32. css_content method is very long, and looks to have a lot of repetitious code. Can't you refactor it? 33. Was it really right to delete the two existing css post-processing funcitions? 34. resolve_image_location also looks like it should be refactored to avoid duplication. 35. In lib/outputrenderers.php, line 327, I think you should use $this->output_empty_tag, to keep the coding style consistent. 36. Please commit the whitespace fixes in webdavlib.php as a separate commit. I hope you don't feel too bad about the number of things I found to comment on. It is mostly excellent, but I think these points need to be addressed before it is ready for CVS.
          Hide
          Petr Škoda added a comment -

          2. backup/restore is dead

          3. plugin stylesheets - this is a bit of misunderstanding - the styles will be in plugins + themes + once more as theme additions in plugins

          4. I had problems with merging - sure, it will be removed everywhere

          5. again merging problems - search replace is going to fix it

          6. yep

          7. yep - login layout

          8. yes - merging trouble

          Show
          Petr Škoda added a comment - 2. backup/restore is dead 3. plugin stylesheets - this is a bit of misunderstanding - the styles will be in plugins + themes + once more as theme additions in plugins 4. I had problems with merging - sure, it will be removed everywhere 5. again merging problems - search replace is going to fix it 6. yep 7. yep - login layout 8. yes - merging trouble
          Hide
          Petr Škoda added a comment -

          9. is a migration process - I just moved everything to standard to make room for new stuff in plugins

          10. this is internal implementation details of themes - it is allowed to do anything with those options; core code is not goign to access these

          11. $THEME does not need to be global, BC only

          12. yes

          13. internal implementation detail - this is not part of API

          14. IE6 needs tons of useless divs, grrr

          15. no, the current themes in standard are just temporary - we need room to work on better sheets

          16. this is not my idea, theme designer had huge problems with undoing stuff from standard theme, they need something very basic when doing new designs

          17. rtl is replaced by direction class

          18. no idea why is fav icon there, I was not sure - pix is better place

          19. yes, custom corners shoudl be there in 2.0, the CVS history is not going to be problem because the style sheets will have different names - no more font+style+color

          20. yes

          21. merging problem -search replace fixes that

          22. I had some problems in installation - the new themes load the pages in different order

          23. grrrr, death to tabs

          Show
          Petr Škoda added a comment - 9. is a migration process - I just moved everything to standard to make room for new stuff in plugins 10. this is internal implementation details of themes - it is allowed to do anything with those options; core code is not goign to access these 11. $THEME does not need to be global, BC only 12. yes 13. internal implementation detail - this is not part of API 14. IE6 needs tons of useless divs, grrr 15. no, the current themes in standard are just temporary - we need room to work on better sheets 16. this is not my idea, theme designer had huge problems with undoing stuff from standard theme, they need something very basic when doing new designs 17. rtl is replaced by direction class 18. no idea why is fav icon there, I was not sure - pix is better place 19. yes, custom corners shoudl be there in 2.0, the CVS history is not going to be problem because the style sheets will have different names - no more font+style+color 20. yes 21. merging problem -search replace fixes that 22. I had some problems in installation - the new themes load the pages in different order 23. grrrr, death to tabs
          Hide
          Petr Škoda added a comment -

          24. anything better than minlib.php is fine for me - can be changed later

          25. yet, a lot more subtasks

          26. who invented the two lines??? ok, ok

          27. I just like lib, it could be placed into other file - can be changed later at any time

          28. I like picking only what is needed - internal implementation detail, but my proposed way looks a bit safer

          29. grrrrrr tabs die dir

          30. -32. ok

          33. one postprocessing looks enough for me - going to review it once more

          34.-- ok

          THANKS A LOT - GREAT FEEDBACK!

          Show
          Petr Škoda added a comment - 24. anything better than minlib.php is fine for me - can be changed later 25. yet, a lot more subtasks 26. who invented the two lines??? ok, ok 27. I just like lib, it could be placed into other file - can be changed later at any time 28. I like picking only what is needed - internal implementation detail, but my proposed way looks a bit safer 29. grrrrrr tabs die dir 30. -32. ok 33. one postprocessing looks enough for me - going to review it once more 34.-- ok THANKS A LOT - GREAT FEEDBACK!
          Hide
          Tim Hunt added a comment -

          A few responses:

          2. Yes, but that commented out code appears in many other places. That was just the first place I saw it. The other places are more important.

          3. Please can you explain how you think it should work on http://docs.moodle.org/en/Development:Theme_changes_proposal

          9. It is a migration process that makes CVS history unnecessarily difficult to track. Why not go directly to styles.css?

          13. $OUTPUT->has_navbar is part of the API that should not be there. I'll file a bug on Sam H. MDL-20833

          16. But surely no-one uses standard theme in production. My point is that we should have one base theme, which uses all the CSS that is in plugins and core outside the theme folder. And then a bunch of nice themes that people might actually want to use, one of which is the default.

          19. Still better to reduce the number of renames etc. that you have to track when looking back through history. Also, if/when we move to git, it can cope well with renames, but it cannot cope with delete then add back later.

          I think a better solution would be to leave these themes in core, but in their config.php do something hacky like set_config('theme', 'standard'); die;

          24. The best I can do is configonlylib.php. That is a bit sucky, but clearer, and it is not a library that people will refer to often.

          28. The point it to make the PHP docs as helpful as possible. I don't see the problem in making the other properties protected with get methods.

          Show
          Tim Hunt added a comment - A few responses: 2. Yes, but that commented out code appears in many other places. That was just the first place I saw it. The other places are more important. 3. Please can you explain how you think it should work on http://docs.moodle.org/en/Development:Theme_changes_proposal 9. It is a migration process that makes CVS history unnecessarily difficult to track. Why not go directly to styles.css? 13. $OUTPUT->has_navbar is part of the API that should not be there. I'll file a bug on Sam H. MDL-20833 16. But surely no-one uses standard theme in production. My point is that we should have one base theme, which uses all the CSS that is in plugins and core outside the theme folder. And then a bunch of nice themes that people might actually want to use, one of which is the default. 19. Still better to reduce the number of renames etc. that you have to track when looking back through history. Also, if/when we move to git, it can cope well with renames, but it cannot cope with delete then add back later. I think a better solution would be to leave these themes in core, but in their config.php do something hacky like set_config('theme', 'standard'); die; 24. The best I can do is configonlylib.php. That is a bit sucky, but clearer, and it is not a library that people will refer to often. 28. The point it to make the PHP docs as helpful as possible. I don't see the problem in making the other properties protected with get methods.
          Hide
          Petr Škoda added a comment -

          3. yes

          9. no, I need to keep them separate for now so that I am able to work on this; each theme is allowed to do whatever it thinks is correct here

          16. we do need the standard theme for now and MD wanted something similar to standard in 2.0, I think it is a good idea to keep it

          19. Old and new themes will share only config.php, keeping the old unused code around breaks my grepping and sanity. I believe there will be very little shared history. The themes are something really new both inside and outside.

          28. theme designers are not developers, I think it is better to not allow them to override the config directly and use it just as a simple object with properties - it is very hard to diagnose OOP problems in code that can not print directly to output. I think that theme developers need something easier to use than PHPDoc.

          Show
          Petr Škoda added a comment - 3. yes 9. no, I need to keep them separate for now so that I am able to work on this; each theme is allowed to do whatever it thinks is correct here 16. we do need the standard theme for now and MD wanted something similar to standard in 2.0, I think it is a good idea to keep it 19. Old and new themes will share only config.php, keeping the old unused code around breaks my grepping and sanity. I believe there will be very little shared history. The themes are something really new both inside and outside. 28. theme designers are not developers, I think it is better to not allow them to override the config directly and use it just as a simple object with properties - it is very hard to diagnose OOP problems in code that can not print directly to output. I think that theme developers need something easier to use than PHPDoc.
          Hide
          Tim Hunt added a comment -

          Petr, please revert your last commit immediately. The

          removed problematic HTML_ATTR_EMPTY, simply use null instead + removed incorrect trimming of values (yes, leading/trailing spaces are sometimes valid attribute values too!)

          one.

          It is very, very useful that in many places you can do things like
          html_writer::tag('div', array('class' => $this->get_class()));
          or something, and know that that class attribute will be omitted if get_class returns something blank.

          It saves about a million if statements all over the place in output code.

          It is very rare to need an something="" attribute - alt="" is the only one I can think of right now, so it is right that that requires an special value, which should probably be html_writer::EMPTY_ATTRIBUTE.

          Show
          Tim Hunt added a comment - Petr, please revert your last commit immediately. The removed problematic HTML_ATTR_EMPTY, simply use null instead + removed incorrect trimming of values (yes, leading/trailing spaces are sometimes valid attribute values too!) one. It is very, very useful that in many places you can do things like html_writer::tag('div', array('class' => $this->get_class())); or something, and know that that class attribute will be omitted if get_class returns something blank. It saves about a million if statements all over the place in output code. It is very rare to need an something="" attribute - alt="" is the only one I can think of right now, so it is right that that requires an special value, which should probably be html_writer::EMPTY_ATTRIBUTE.
          Hide
          Petr Škoda added a comment -

          sorry tim, please use real NULL as a default value, this HTML_ATTR_EMPTY was breaking other stuff because empty string is a valid value in cases like html select options. Please let's not do the same mistake like Oracle - empty string and NULL is something really different, so let's use it properly.

          BTW while working on this I discovered wrong handling of disabled attributes and other similar issues.

          Show
          Petr Škoda added a comment - sorry tim, please use real NULL as a default value, this HTML_ATTR_EMPTY was breaking other stuff because empty string is a valid value in cases like html select options. Please let's not do the same mistake like Oracle - empty string and NULL is something really different, so let's use it properly. BTW while working on this I discovered wrong handling of disabled attributes and other similar issues.
          Hide
          Petr Škoda added a comment -

          also I discovered one scary trim($value) which I suppose had the same origin like the HTML_ATTR_EMPTY

          please file any regression into tracker and I will fix them asap

          Show
          Petr Škoda added a comment - also I discovered one scary trim($value) which I suppose had the same origin like the HTML_ATTR_EMPTY please file any regression into tracker and I will fix them asap
          Hide
          Tim Hunt added a comment -

          Sorry Petr, how is it a good idea to force people to write code like:

          $attributes = array(...);
          $class = $this->get_class();
          if ($class)

          { $attributes ['class'] = $class; }

          html_writer::tag('div', $attributes);

          When it could be

          html_writer::tag('div', array(..., $this->get_class()));

          Surely you agree that the second is a nicer API for developers.

          I can understand why, using the Oracle parallel, you have some doubts, but I think that on balance, convenience 99% of the time is worth it.

          Show
          Tim Hunt added a comment - Sorry Petr, how is it a good idea to force people to write code like: $attributes = array(...); $class = $this->get_class(); if ($class) { $attributes ['class'] = $class; } html_writer::tag('div', $attributes); When it could be html_writer::tag('div', array(..., $this->get_class())); Surely you agree that the second is a nicer API for developers. I can understand why, using the Oracle parallel, you have some doubts, but I think that on balance, convenience 99% of the time is worth it.
          Hide
          Petr Škoda added a comment - - edited

          you still can write html_writer::tag('div', array(..., $this->get_class())); if your get_class() returns null, alternatively we could intelligently omit some known empty attributes such as class, but redefining empty string to null and HTML_ATTR_EMPTY to empty string is definitely not a good technical solution. Your HTML_ATTR_EMPTY was breaking my code pretty badly and there was no workaround, I strongly disagree that it is useful in 99% of cases, I spent a lot of time in this area recently...

          Show
          Petr Škoda added a comment - - edited you still can write html_writer::tag('div', array(..., $this->get_class())); if your get_class() returns null, alternatively we could intelligently omit some known empty attributes such as class, but redefining empty string to null and HTML_ATTR_EMPTY to empty string is definitely not a good technical solution. Your HTML_ATTR_EMPTY was breaking my code pretty badly and there was no workaround, I strongly disagree that it is useful in 99% of cases, I spent a lot of time in this area recently...
          Hide
          Tim Hunt added a comment -

          Actually, I worked out the answer is:

          fuction opt($value) {
              if ($value !== '0' && !$value) {
                  return null;
              }
              return $value;
          }
          

          Then

          html_writer::tag('div', array(..., opt($this->get_class())));

          That should make both of us happy.

          Show
          Tim Hunt added a comment - Actually, I worked out the answer is: fuction opt($value) { if ($value !== '0' && !$value) { return null ; } return $value; } Then html_writer::tag('div', array(..., opt($this->get_class()))); That should make both of us happy.
          Hide
          Matt Gibson added a comment -

          I just ran into a big headache trying to do some theming stuff and finding that the css didn't change when I emptied the browser cache.

          It took me a while to find the button to reset the theme cache because it doesn't show up when you search the admin block. It's also not where I expected it - I was looking in the performance section.

          I think one or both of those things needs changing so that it's easier to find.

          Show
          Matt Gibson added a comment - I just ran into a big headache trying to do some theming stuff and finding that the css didn't change when I emptied the browser cache. It took me a while to find the button to reset the theme cache because it doesn't show up when you search the admin block. It's also not where I expected it - I was looking in the performance section. I think one or both of those things needs changing so that it's easier to find.
          Hide
          Petr Škoda added a comment -

          done long time ago, thanks everybody...

          Show
          Petr Škoda added a comment - done long time ago, thanks everybody...

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: