Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Badges, Themes
    • Labels:
    • Testing Instructions:
      Hide

      Testing would involve changing current theme to a bootstrap theme and looking through the badges pages to make sure that everything looks reasonable.

      Pay attention to "My badges" page under "My profile", as it should display badges in a grid like view.

      All tables (e.g. on pages Manage badges and badge recipients) should be striped. Colors should follow Moodle color scheme.

      Show
      Testing would involve changing current theme to a bootstrap theme and looking through the badges pages to make sure that everything looks reasonable. Pay attention to "My badges" page under "My profile", as it should display badges in a grid like view. All tables (e.g. on pages Manage badges and badge recipients) should be striped. Colors should follow Moodle color scheme.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      49760

      Description

      We need to move the base css for badges into the boostrap base theme.

      Additionally I think some of the CSS rules might need tightening (one which caught my eye was:

      .connected { color: #006600; }
      .notconnected { color: #660000; }
      

      But i'm sure there are a few more which might be best tied down more specifically. (Having said that, i'm not a theme developer, so..)

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Adding Mary/David/Bas here.

          Show
          Dan Poltawski added a comment - Adding Mary/David/Bas here.
          Hide
          Yuliya Bozhko added a comment -

          I might need a bit of help with this one.

          Does it mean adding badges to the bootstrap theme from MDL-38016?

          If yes, as I understand from the instructions at https://github.com/bmbrands/theme_bootstrap/tree/moodle_25 CSS is being generated and should not be edited manually?

          Show
          Yuliya Bozhko added a comment - I might need a bit of help with this one. Does it mean adding badges to the bootstrap theme from MDL-38016 ? If yes, as I understand from the instructions at https://github.com/bmbrands/theme_bootstrap/tree/moodle_25 CSS is being generated and should not be edited manually?
          Hide
          Mary Evans added a comment - - edited

          Yuliya you can add the CSS to theme/bootstrap/less/moodle/ in the same way Marina added some course related CSS in MDL-39064 perhaps you should ask her how this all works?

          I can forsee problems with conflicts especially if other CSS changes/additions are taking place in other tracker issues.

          Perhaps it may be better to just add the CSS and let David Scotson generate the new CSS when all CSS has been added?

          Or alternatively all CSS added at the same time in one tracker issue?

          Show
          Mary Evans added a comment - - edited Yuliya you can add the CSS to theme/bootstrap/less/moodle/ in the same way Marina added some course related CSS in MDL-39064 perhaps you should ask her how this all works? I can forsee problems with conflicts especially if other CSS changes/additions are taking place in other tracker issues. Perhaps it may be better to just add the CSS and let David Scotson generate the new CSS when all CSS has been added? Or alternatively all CSS added at the same time in one tracker issue?
          Hide
          David Scotson added a comment - - edited

          Porting CSS into LESS for Bootstrap is a three step process:

          1. Decide where to add it. Instead of theme/base/style/something.css it'll be something in theme/bootstrap/less/moodle/something.less instead. Some of the file names map over directly, but others have been tidied up or split into more manageable chunks. I see that, like most new CSS in Moodle this is just added at the bottom of core.css. Ideally since there's a fair amount of CSS it would be added to a new file called badges.less but that introduces complications, so add it at the bottom of theme/bootstrap/less/core.less

          2. Add it. This is the easy bit. You can cut and paste 99.99% of valid CSS into a less file and it'll just work.

          3. Regenerate the generated.css. There are instructions in the wiki (http://docs.moodle.org/dev/LESS), if those are unclear then it would be good to get feedback so we can improve them. They've only just started so they're a bit bare bones, installing npm and node is the hard part but you may have already done that if you're working with javascript. Let us know if anything is confusing, missing or wrong.

          You should end up with changes to the core.less file and the generated.css file to check in.

          Show
          David Scotson added a comment - - edited Porting CSS into LESS for Bootstrap is a three step process: 1. Decide where to add it. Instead of theme/base/style/ something .css it'll be something in theme/bootstrap/less/moodle/ something .less instead. Some of the file names map over directly, but others have been tidied up or split into more manageable chunks. I see that, like most new CSS in Moodle this is just added at the bottom of core.css. Ideally since there's a fair amount of CSS it would be added to a new file called badges.less but that introduces complications, so add it at the bottom of theme/bootstrap/less/core.less 2. Add it. This is the easy bit. You can cut and paste 99.99% of valid CSS into a less file and it'll just work. 3. Regenerate the generated.css. There are instructions in the wiki ( http://docs.moodle.org/dev/LESS ), if those are unclear then it would be good to get feedback so we can improve them. They've only just started so they're a bit bare bones, installing npm and node is the hard part but you may have already done that if you're working with javascript. Let us know if anything is confusing, missing or wrong. You should end up with changes to the core.less file and the generated.css file to check in.
          Hide
          David Scotson added a comment -

          Regarding the actual CSS itself, there's a few issues but that's a separate matter from porting to Bootstrap and they're general issues that apply to basically all the CSS in Moodle (e.g. the formatting, the way table stripes are applied, use of !important, "white" instead of #fff etc.) Hopefully we can fix them all up later.

          A few LESS specific ones if you feel like addressing them:

          • You can use an opacity mixin instead of all those repeated lines (".opacity(85)") see other examples of it's use in core.less.
          • Those red and green colors should be color: @successText; and color: @errorText; to pick up the Bootstrap colors.
          • A bunch of the table styling could just be removed. As long as the table has the .generaltable class it'll get striping and some other stuff for free.

          Any questions let me know,

          dave

          Show
          David Scotson added a comment - Regarding the actual CSS itself, there's a few issues but that's a separate matter from porting to Bootstrap and they're general issues that apply to basically all the CSS in Moodle (e.g. the formatting, the way table stripes are applied, use of !important, "white" instead of #fff etc.) Hopefully we can fix them all up later. A few LESS specific ones if you feel like addressing them: You can use an opacity mixin instead of all those repeated lines (".opacity(85)") see other examples of it's use in core.less. Those red and green colors should be color: @successText; and color: @errorText; to pick up the Bootstrap colors. A bunch of the table styling could just be removed. As long as the table has the .generaltable class it'll get striping and some other stuff for free. Any questions let me know, dave
          Hide
          Yuliya Bozhko added a comment -

          Hi Dave,

          Thanks for your help. I looked at the docs http://docs.moodle.org/dev/LESS and instructions are quite good. Just a couple things: I don't think there is a need for both "sudo apt-get install npm node", when you install npm only, node is installed by default (I accidentally got into the loop of adding and removing packages ). Another issue that I had was that "npm install recess -g" required admin permissions and for me worked only under sudo.

          One last thing, do I submit a patch with both core.less and generated.css or will CSS file be generated by you when all CSS has been added to bootstrap theme (as Mary suggested)?

          Show
          Yuliya Bozhko added a comment - Hi Dave, Thanks for your help. I looked at the docs http://docs.moodle.org/dev/LESS and instructions are quite good. Just a couple things: I don't think there is a need for both " sudo apt-get install npm node ", when you install npm only, node is installed by default (I accidentally got into the loop of adding and removing packages ). Another issue that I had was that " npm install recess -g " required admin permissions and for me worked only under sudo . One last thing, do I submit a patch with both core.less and generated.css or will CSS file be generated by you when all CSS has been added to bootstrap theme (as Mary suggested)?
          Hide
          Dan Poltawski added a comment -

          Hi Yuliya,

          Please can you submit a path with both - thanks.

          Show
          Dan Poltawski added a comment - Hi Yuliya, Please can you submit a path with both - thanks.
          Hide
          Yuliya Bozhko added a comment -

          I added badges to bootstrap. Sorry, if it still looks weird in some places, I am not a designer

          Show
          Yuliya Bozhko added a comment - I added badges to bootstrap. Sorry, if it still looks weird in some places, I am not a designer
          Hide
          Dan Poltawski added a comment -

          David Scotson could you peer review this?

          Show
          Dan Poltawski added a comment - David Scotson could you peer review this?
          Hide
          David Scotson added a comment - - edited

          [Y] Syntax
          [-] Output
          [Y] Whitespace *
          [-] Language
          [-] Databases
          [-] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          • There was one line for .badge-profile that wasn't indented correctly, and a width and height on the same line a couple of lines above that. It's not a big deal, the CSS/LESS generally needs re-formatted, but if you're making other changes you could fix those too.

          The only parts of the port I'd suggest changing are a) these colors (which are currently hardcoded):

          .statusbox.active {
              background-color: @successBackground;
          }
          .statusbox.inactive {
              background-color: @warningBackground;
          }
          

          The first just changes the shade of green to match other similar uses in Bootstrap.

          The second changes the yellow similarly. I was going to suggest changing it to red to match similar messages elsewhere in Moodle telling you that something is switched off, but then I realised that you can't edit it unless it's off so there's a bit of a mixed message here so just leave it as "yellow/warning"

          and b) the table styles for adding borders and stripes, which can be replaced by the following in .less to pick up the standard look for a bordered, striped, table.

          table.collection {
              .table;
              .table-bordered;
              .table-striped;
          }
          

          Some issues unrelated to the port to Bootstrap I noticed along the way:

          There's some odd uses of tables to center that statusbox.

          The sortable table where you "manage badges" only links the sort arrow image, not the whole text. And the badge one shows both the sort up and sort down arrows at the same time. I think there's library code in Moodle for this that acts differently from this. There's similar stuff happening on the list of recipients.

          I'm not sure the fact that the badge is active needs to be permanently highlighted, just the lack of an warning that it's off should be enough (though the fact you can't edit when they're on complicates this).

          I'm guessing the reason for the permanent highlighting is something to do with having to turn the badge off before editing it. The highlight bar itself only talks about making it unavailable. The help text talks about the badge becoming locked after being awarded, but that didn't seem to happen when I gave someone a badge and then edited it.

          The table used for the badge overview should have th tags for the headers on the left hand side. (In fact there should be a library in Moodle for creating these two column "tables", probably using the same CSS as the form layouts, but currently there isn't as far as I'm aware).

          Show
          David Scotson added a comment - - edited [Y] Syntax [-] Output [Y] Whitespace * [-] Language [-] Databases [-] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check There was one line for .badge-profile that wasn't indented correctly, and a width and height on the same line a couple of lines above that. It's not a big deal, the CSS/LESS generally needs re-formatted, but if you're making other changes you could fix those too. The only parts of the port I'd suggest changing are a) these colors (which are currently hardcoded): .statusbox.active { background-color: @successBackground; } .statusbox.inactive { background-color: @warningBackground; } The first just changes the shade of green to match other similar uses in Bootstrap. The second changes the yellow similarly. I was going to suggest changing it to red to match similar messages elsewhere in Moodle telling you that something is switched off, but then I realised that you can't edit it unless it's off so there's a bit of a mixed message here so just leave it as "yellow/warning" and b) the table styles for adding borders and stripes, which can be replaced by the following in .less to pick up the standard look for a bordered, striped, table. table.collection { .table; .table-bordered; .table-striped; } Some issues unrelated to the port to Bootstrap I noticed along the way: There's some odd uses of tables to center that statusbox. The sortable table where you "manage badges" only links the sort arrow image, not the whole text. And the badge one shows both the sort up and sort down arrows at the same time. I think there's library code in Moodle for this that acts differently from this. There's similar stuff happening on the list of recipients. I'm not sure the fact that the badge is active needs to be permanently highlighted, just the lack of an warning that it's off should be enough (though the fact you can't edit when they're on complicates this). I'm guessing the reason for the permanent highlighting is something to do with having to turn the badge off before editing it. The highlight bar itself only talks about making it unavailable. The help text talks about the badge becoming locked after being awarded, but that didn't seem to happen when I gave someone a badge and then edited it. The table used for the badge overview should have th tags for the headers on the left hand side. (In fact there should be a library in Moodle for creating these two column "tables", probably using the same CSS as the form layouts, but currently there isn't as far as I'm aware).
          Hide
          David Scotson added a comment -

          One more thing:

          Tables in Moodle Standard theme inherit 0.5em padding from YUI. Bootstrapbase doesn't include YUI, so start with 0 padding. This is causing the badge overview titles to but right up against the values. Applying the .table styles partly works, but makes them wider than ideal and has too many lines.

          There's no other easy way to target these tables via CSS to add the padding back to the first cell. I'm wondering if just adding this style back globally might be the easiest way to transition?

          Show
          David Scotson added a comment - One more thing: Tables in Moodle Standard theme inherit 0.5em padding from YUI. Bootstrapbase doesn't include YUI, so start with 0 padding. This is causing the badge overview titles to but right up against the values. Applying the .table styles partly works, but makes them wider than ideal and has too many lines. There's no other easy way to target these tables via CSS to add the padding back to the first cell. I'm wondering if just adding this style back globally might be the easiest way to transition?
          Hide
          Yuliya Bozhko added a comment -

          Hi David,

          I did the changes to the bootstrap port the ones that you have recommended. At some point I will need to look through the rest of the stuff and make sure that it is fixed, but it is not related to this patch and I will fix it separately. Thanks a lot for your help!

          Show
          Yuliya Bozhko added a comment - Hi David, I did the changes to the bootstrap port the ones that you have recommended. At some point I will need to look through the rest of the stuff and make sure that it is fixed, but it is not related to this patch and I will fix it separately. Thanks a lot for your help!
          Hide
          Martin Dougiamas added a comment -

          Yuliya can you add some testing instructions? What does a tester need to look at to check this is ok?

          Show
          Martin Dougiamas added a comment - Yuliya can you add some testing instructions? What does a tester need to look at to check this is ok?
          Hide
          Damyon Wiese added a comment -

          I found a formatting issue when testing this in integration - on the mybadges page the clear button is not aligned with the search button.

          The reason is that bootstrap adds rules for buttons/submits etc following a text input so that the buttons/submits get extra margins to make them line up. But they don't allow for 2 buttons/submits etc following a text input. No be honest I don't think that button is needed at all and my solution would be to remove it (why isn't it a 'reset' button anyway?).

          I'll add a new issue for that.

          Show
          Damyon Wiese added a comment - I found a formatting issue when testing this in integration - on the mybadges page the clear button is not aligned with the search button. The reason is that bootstrap adds rules for buttons/submits etc following a text input so that the buttons/submits get extra margins to make them line up. But they don't allow for 2 buttons/submits etc following a text input. No be honest I don't think that button is needed at all and my solution would be to remove it (why isn't it a 'reset' button anyway?). I'll add a new issue for that.
          Hide
          Yuliya Bozhko added a comment -

          Thanks, Damyon! Will fix it in a new issue

          Show
          Yuliya Bozhko added a comment - Thanks, Damyon! Will fix it in a new issue
          Hide
          Damyon Wiese added a comment -

          Thanks Yuliya, this issue has been integrated to master.

          Show
          Damyon Wiese added a comment - Thanks Yuliya, this issue has been integrated to master.
          Hide
          Andrew Davis added a comment -

          Looks great. Passing.

          Show
          Andrew Davis added a comment - Looks great. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: