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 2.6 Branch:
    • 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

          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

                  Agile