Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41767

plugins unable to provide "bootstrapbase" or "clean" styles

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.5, 2.6
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Locate a theme which as 2 parent themes ($THEME->parents)
      2. In a plugin which defines a styles.css pick a simple rule from the file styles.css
        • Example: .mod_blah h2 { font-size: 13px; }
      3. Creates 3 additional files
        • styles_<parent1>.css
          • Only containing: .mod_blah h2 { font-size: 14px; }
        • styles_<parent2>.css
          • Only containing: .mod_blah h2 { font-size: 15px; }
        • styles_<theme_chosen>.css
          • Only containing: .mod_blah h2 { font-size: 16px; }
      4. Now visit the module you and using FireBug locate the element that you have been styling
      5. Make sure that each of the rules have been used, but only the one from styles_<theme_chosen>.css is effective.
      Show
      Locate a theme which as 2 parent themes ($THEME->parents) In a plugin which defines a styles.css pick a simple rule from the file styles.css Example: .mod_blah h2 { font-size: 13px; } Creates 3 additional files styles_<parent1>.css Only containing: .mod_blah h2 { font-size: 14px; } styles_<parent2>.css Only containing: .mod_blah h2 { font-size: 15px; } styles_<theme_chosen>.css Only containing: .mod_blah h2 { font-size: 16px; } Now visit the module you and using FireBug locate the element that you have been styling Make sure that each of the rules have been used, but only the one from styles_<theme_chosen>.css is effective.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41767-master
    • Sprint:
      FRONTEND Sprint 10
    • Sprint:
      FRONTEND Sprint 10

      Description

      Given the mod_grrr contrib plugin (or subplugin) as example, right now it's possible to specify a custom css sheet:

      mod/grr/styles.css

      And also it's possible to specify custom css by current theme (assume standard):

      mod/grrr/styles_standard.css

      The 1st is used everywhere (both in core and contrib plugins). And the 2nd is way less common, but still has a point in some rare situations.

      But it's impossible to point to the parent themes so this does not work:

      mod/grrr/styles_base.css

      With the arrival of the "base" and "bootstrapbase" duality in 2.5, and more if the later is going to become the "default" one… we need to be able to specify any of these, per plugin:

      • mod/grrr/styles.css (to be applies to all themes)
      • mod/grrr/styles_base.css (to be applied to all "base" themes)
      • mod/grrr/styles_bootstrapbase.css (to be applied to all "bootstrapbase" themes)
      • mod/grrr/styles_standard.css (to be applies to "standard", and any "child" of it.
      • mod/grrr/styles_clean.css (to be applied to "clean", and any "child" of it).

      I other words, we need to add support to parent themes in css_files() also for plugins, if I'm not wrong. Right now only the "current" theme is supported. We should be looking for the existence of any parent there.

      That's the only way I can imagine to keep things organized. Right now we are using a modules.less (and .css) in bootstrapbase but it has 2 limitations:

      1) It only works for core plugins.
      2) It is against the whole organization of plugins (self-contained).

      So this is about to implement and document the support of parent themes css for plugins.

      Ciao

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              (raising it at least to major, I think this is something we need to be addressed asap)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - (raising it at least to major, I think this is something we need to be addressed asap)
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              For reference, changes here will imply to modify/complete/clarify:

              http://docs.moodle.org/dev/Themes_overview#Locations_of_CSS_files

              (and maybe others there)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited For reference, changes here will imply to modify/complete/clarify: http://docs.moodle.org/dev/Themes_overview#Locations_of_CSS_files (and maybe others there)
              Hide
              lazydaisy Mary Evans added a comment - - edited

              Please forgive me if I am wrong, but why on earth should it be necessary to style a plug-in for every theme?

              A mod should be neutral in colour yet fit in with the general look of Moodle. Any styles can go into base if necessary and also in bootstrapbase, unless the mod is styled using bootstrap css selectors then there would be no need to style it as the css is there reading for the picking, meaning, that bootstrap would style it for free! LOL

              Show
              lazydaisy Mary Evans added a comment - - edited Please forgive me if I am wrong, but why on earth should it be necessary to style a plug-in for every theme? A mod should be neutral in colour yet fit in with the general look of Moodle. Any styles can go into base if necessary and also in bootstrapbase, unless the mod is styled using bootstrap css selectors then there would be no need to style it as the css is there reading for the picking, meaning, that bootstrap would style it for free! LOL
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              Mary, you're correct. Styling exact themes from plugins has no sense 99.99% of times but, curiously, that's already supported since ages ago (mod/grrr/styles_magazine.css) for current theme.

              I just wanted to generalize that existing support so we can reach base themes (base and bootstrap), because I'm sure there are going to be more cases needing that. So, if current theme is magazine, all these are looked and added when existing:

              • styles.css
              • styles_base.css
              • styles_standard.css
              • styles_magazine.css (assuming magazine has standard as parent, to enrich the example)

              (right now only the 1st and the 4th are supported, I want to add support for 2nd and 3rd, knowing that the important one is the 2nd, aka bases).

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited Mary, you're correct. Styling exact themes from plugins has no sense 99.99% of times but, curiously, that's already supported since ages ago (mod/grrr/styles_magazine.css) for current theme. I just wanted to generalize that existing support so we can reach base themes (base and bootstrap), because I'm sure there are going to be more cases needing that. So, if current theme is magazine, all these are looked and added when existing: styles.css styles_base.css styles_standard.css styles_magazine.css (assuming magazine has standard as parent, to enrich the example) (right now only the 1st and the 4th are supported, I want to add support for 2nd and 3rd, knowing that the important one is the 2nd, aka bases). Ciao
              Hide
              lazydaisy Mary Evans added a comment - - edited

              Eloy, there are NO themes in Moodle that use Standard theme as a parent. ALL themes use Base theme as either parent or grandparent. So Base theme ALWAYS fills in the missing CSS.

              If anything numero 3 should be styles_canvas.css as 75% of all Moodle themes use Canvas as parent.

              Bootstrap is different, hence my mention of using Bootstrap CSS selectors in mods/plug-ins.

              I did not know that mods/plug-ins could style specifically for a theme, now that is useful to know.

              Show
              lazydaisy Mary Evans added a comment - - edited Eloy, there are NO themes in Moodle that use Standard theme as a parent. ALL themes use Base theme as either parent or grandparent. So Base theme ALWAYS fills in the missing CSS. If anything numero 3 should be styles_canvas.css as 75% of all Moodle themes use Canvas as parent. Bootstrap is different, hence my mention of using Bootstrap CSS selectors in mods/plug-ins. I did not know that mods/plug-ins could style specifically for a theme, now that is useful to know.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              Mary, it was an example, just an example of parentship (hence the "assuming magazine has standard as parent, to enrich the example").

              Not really sure what are we discussing here, it's a simple as, given a theme, look for all its parents up to grand one(s) (those are the base(s)). As simple as that. No matter if there is one parent, 10 parents or 200 parents.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited Mary, it was an example, just an example of parentship (hence the "assuming magazine has standard as parent, to enrich the example"). Not really sure what are we discussing here, it's a simple as, given a theme, look for all its parents up to grand one(s) (those are the base(s)). As simple as that. No matter if there is one parent, 10 parents or 200 parents. Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              In other words, exactly the same we are doing for main sheets in themes (top-down):

              https://github.com/moodle/moodle/blob/master/lib/outputlib.php#L799

              But within a plugin, with themes being the part of the name of the sheet:

              styles_$themename.css

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - In other words, exactly the same we are doing for main sheets in themes (top-down): https://github.com/moodle/moodle/blob/master/lib/outputlib.php#L799 But within a plugin, with themes being the part of the name of the sheet: styles_$themename.css Ciao
              Hide
              lazydaisy Mary Evans added a comment -

              After I posted my last comment above, I realised that what you are trying to do. So yes...I agree we need to do this if we ever want to utilise this behaviour.

              Show
              lazydaisy Mary Evans added a comment - After I posted my last comment above, I realised that what you are trying to do. So yes...I agree we need to do this if we ever want to utilise this behaviour.
              Hide
              fred Frédéric Massart added a comment -

              Sending this for peer review. While I strongly believe that we should never ever write theme/parent specific styles in plugins, I agree that this is a bug.

              Show
              fred Frédéric Massart added a comment - Sending this for peer review. While I strongly believe that we should never ever write theme/parent specific styles in plugins, I agree that this is a bug.
              Hide
              fred Frédéric Massart added a comment -

              This reminded me of MDL-42961.

              Show
              fred Frédéric Massart added a comment - This reminded me of MDL-42961 .
              Hide
              cibot CiBoT added a comment -

              Results for MDL-41767

              • Remote repository: git://github.com/FMCorz/moodle.git
              Show
              cibot CiBoT added a comment - Results for MDL-41767 Remote repository: git://github.com/FMCorz/moodle.git Remote branch MDL-41767 -25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/658 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/658/artifact/work/smurf.html Remote branch MDL-41767 -26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/659 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/659/artifact/work/smurf.html Remote branch MDL-41767 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/660 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/660/artifact/work/smurf.html
              Hide
              poltawski Dan Poltawski added a comment -

              Fred, if you don't get a review on this within the next hour, please push this for review. I feel you can get 'component maintainer' priviledges and push this as its been waiting for 1 month.

              Show
              poltawski Dan Poltawski added a comment - Fred, if you don't get a review on this within the next hour, please push this for review. I feel you can get 'component maintainer' priviledges and push this as its been waiting for 1 month.
              Hide
              fred Frédéric Massart added a comment -

              Thanks Dan, pushing for integration.

              Show
              fred Frédéric Massart added a comment - Thanks Dan, pushing for integration.
              Hide
              lazydaisy Mary Evans added a comment - - edited

              @Fred, will this also affect things like Course Formats or are they classed as a different type of plugin?

              Show
              lazydaisy Mary Evans added a comment - - edited @Fred, will this also affect things like Course Formats or are they classed as a different type of plugin?
              Hide
              timhunt Tim Hunt added a comment -

              Mary, looking at the patch, this works for all plugin types (except themes!).

              Show
              timhunt Tim Hunt added a comment - Mary, looking at the patch, this works for all plugin types (except themes!).
              Hide
              lazydaisy Mary Evans added a comment - - edited

              Oh that's OK then...themes can exclude plugins CSS sheets. LOL

              Show
              lazydaisy Mary Evans added a comment - - edited Oh that's OK then...themes can exclude plugins CSS sheets. LOL
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Fred,

              This seems OK - soon people will ask to be able to provide .less files in plugins .

              I tested it and it works - I tested that $THEME->plugins_exclude_sheets works with this setting too (after I fixed the docs for it).

              Integrated to 25, 26 and master.

              Show
              damyon Damyon Wiese added a comment - Thanks Fred, This seems OK - soon people will ask to be able to provide .less files in plugins . I tested it and it works - I tested that $THEME->plugins_exclude_sheets works with this setting too (after I fixed the docs for it). Integrated to 25, 26 and master.
              Hide
              damyon Damyon Wiese added a comment -

              I added dev_docs_required here too. Can you just check the docs and make sure this is clearly explained somewhere?

              Thanks!

              Show
              damyon Damyon Wiese added a comment - I added dev_docs_required here too. Can you just check the docs and make sure this is clearly explained somewhere? Thanks!
              Hide
              fred Frédéric Massart added a comment -

              Thanks Damyon, I added the doc link.

              Show
              fred Frédéric Massart added a comment - Thanks Damyon, I added the doc link.
              Hide
              dmonllao David Monllaó added a comment -

              It passes. All works as described. Tested in 25, 26 and master

              Show
              dmonllao David Monllaó added a comment - It passes. All works as described. Tested in 25, 26 and master
              Hide
              bawjaws David Scotson added a comment -

              Do all legacy themes inherit from "base", even if it's via Canvas in some cases? If so then it might make sense to have a policy where, rather than writing a styles file that targets Bootstrapbase, you instead move the legacy CSS needed for Base into a styles_base.css. As Mary says, in an ideal world, plugins wouldn't need to be writing custom styles for themselves, as they'd be using standard components and styles used throughout Moodle.

              The two are effectively identical in impact, but it sets the direction a bit more clearly. (I've suggested similar for renderers, where core renderers still target Base themes and then has renderers in Bootstrapbase to overwrite the core ones. The current core renderers could be moved to base and the bootstrapbase renderers moved into core instead. Same code, same effect, but signals which is the priority.)

              Show
              bawjaws David Scotson added a comment - Do all legacy themes inherit from "base", even if it's via Canvas in some cases? If so then it might make sense to have a policy where, rather than writing a styles file that targets Bootstrapbase, you instead move the legacy CSS needed for Base into a styles_base.css. As Mary says, in an ideal world, plugins wouldn't need to be writing custom styles for themselves, as they'd be using standard components and styles used throughout Moodle. The two are effectively identical in impact, but it sets the direction a bit more clearly. (I've suggested similar for renderers, where core renderers still target Base themes and then has renderers in Bootstrapbase to overwrite the core ones. The current core renderers could be moved to base and the bootstrapbase renderers moved into core instead. Same code, same effect, but signals which is the priority.)
              Hide
              damyon Damyon Wiese added a comment -

              Parallel programmParallel programming is harding is hard

              Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

              Show
              damyon Damyon Wiese added a comment - Parallel programmParallel programming is harding is hard Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Mar/14