Details

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

      The menus in the settings and nav blocks should now be 99% identical to the Standard themes.

      Minor differences include: the font, styling of the HR and other general Bootstrap styles that affect the block. But those aside, if there's a difference in behaviour or appearance between this and Standard then it's a bug.

      Show
      The menus in the settings and nav blocks should now be 99% identical to the Standard themes. Minor differences include: the font, styling of the HR and other general Bootstrap styles that affect the block. But those aside, if there's a difference in behaviour or appearance between this and Standard then it's a bug.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-39138-master
    • Rank:
      49733

      Description

      See screenshots comparing the two, but BS is missing the icons for navigation nodes. This gives a really jagged look which is less than pleasing to the eye

      1. awesome.png
        38 kB
      2. awesome.png
        41 kB
      3. bonfire-screenshot-20130419-154104-433.png
        29 kB
      4. classic.png
        39 kB
      5. classic.png
        23 kB
      6. Screen Shot 2013-04-15 at 17.29.06.png
        28 kB
      7. Screen Shot 2013-04-15 at 17.31.38.png
        26 kB

        Issue Links

          Activity

          Hide
          Mary Evans added a comment - - edited

          I think you will find that that was the intention.
          However, to get the Icons to work there is a fare amount of rendering to be done and so I think this was the easy option.

          If you look at the Moodle version of a Bootstrap theme I built based on the original work done on the Bootstrap theme, you will see that has the Bootstrap icons.

          https://github.com/lazydaisy/tiny-bootstrap-project.git

          Screenshot: https://github.com/lazydaisy/tiny-bootstrap-project/blob/master/pix/screenshot.jpg

          Show
          Mary Evans added a comment - - edited I think you will find that that was the intention. However, to get the Icons to work there is a fare amount of rendering to be done and so I think this was the easy option. If you look at the Moodle version of a Bootstrap theme I built based on the original work done on the Bootstrap theme, you will see that has the Bootstrap icons. https://github.com/lazydaisy/tiny-bootstrap-project.git Screenshot: https://github.com/lazydaisy/tiny-bootstrap-project/blob/master/pix/screenshot.jpg
          Hide
          Michael de Raadt added a comment -

          As this is intended, but could change later, I've put this on the DEV backlog, but the decision is your's, Mary.

          Show
          Michael de Raadt added a comment - As this is intended, but could change later, I've put this on the DEV backlog, but the decision is your's, Mary.
          Hide
          Martin Dougiamas added a comment -

          Actually I'd call this a bug.

          A quick fix might be to re-establish the indenting at least so that the items line up, but I think it makes sense to go the whole hog and bring back the icons.

          Mary since you did the work in your version would it be too hard to port it to core?

          Show
          Martin Dougiamas added a comment - Actually I'd call this a bug. A quick fix might be to re-establish the indenting at least so that the items line up, but I think it makes sense to go the whole hog and bring back the icons. Mary since you did the work in your version would it be too hard to port it to core?
          Hide
          Mary Evans added a comment -

          It should be easy enough to add the renderer. I'll work on this now and if it works OK will submit for integration.

          Show
          Mary Evans added a comment - It should be easy enough to add the renderer. I'll work on this now and if it works OK will submit for integration.
          Hide
          Mary Evans added a comment - - edited

          Added renderer and deleted the CSS that adds the arrows to the block tree nav. Enabled the block stylesheet in bootstrap/config.php where it was previously excluded. And still cannot see indents or images. There must be some other changes that Bas/David have done that I can't find.

          Not sure where to look!

          Show
          Mary Evans added a comment - - edited Added renderer and deleted the CSS that adds the arrows to the block tree nav. Enabled the block stylesheet in bootstrap/config.php where it was previously excluded. And still cannot see indents or images. There must be some other changes that Bas/David have done that I can't find. Not sure where to look!
          Hide
          Mary Evans added a comment -

          Adding David Scotson as a watcher hoping he can shed light on this.

          Show
          Mary Evans added a comment - Adding David Scotson as a watcher hoping he can shed light on this.
          Hide
          David Scotson added a comment -

          One of the innovations in Bootstrap is the icons which come in two flavours, black (very dark gray actually) and white. (Actually in Bootstrap 3, or when using the popular Font Awesome, they're an icon font so you can choose any color). When used in a situation with highlighting hovers like a nav list (which is what we copied for the nav and settings block) they switch between the two colors as appropriate. You can see this in the top nav bar in Mary's screenshot or more relevantly to this dicusssion this example of a nav menu from the bootstrap docs (you might have to scroll down from that anchor to the one titled "Navigation")

          http://twitter.github.io/bootstrap/base-css.html#icons

          Moodle's 2.4 action icons are a single color, mid gray. Which works okay against a white or pale gray background but looks a bit odd when put against the blue highlight background, which was the primary motivation for hiding them. It also inspired me to file a bug about converting them to an icon font, see MDL-37231.

          Regarding how you line up things when only some have an icon, there was a bug in Bootstrap's github bug tracker about this though I can't Google it up right now. Several people suggested that the words need to line up even if some don't have icons. The Bootstrap guys pointed to Mac OS X menus which always line up against the far left even if some don't have icons. I followed that for the submenu icons.

          I initally had the submenus so that they didn't exactly line up with the text from the parent branch but stepped in slightly, but it just looked untidy, as if it was supposed to line up but missed.

          So generally, I tried quite a few different combinations and thought this was the most visually pleasing taking into account a variety of factors and constraints. If you think something else would look or act better then it's relatively easy to mockup using browser tools and share a screenshot. The icons img tags are still there (I do have a renderer that omits them but the code in the nav and settings blocks has off the charts complexity so I've not submitted it) so you can just delete the display: none on the image from the CSS to bring them back, or change it to visibility: hidden to see how it looks with an icon sized empty space where the icon should be. (You might need to set the width to 16px and add padding-right: to 3px or so as well depending on how and what you want to line up with).

          Show
          David Scotson added a comment - One of the innovations in Bootstrap is the icons which come in two flavours, black (very dark gray actually) and white. (Actually in Bootstrap 3, or when using the popular Font Awesome, they're an icon font so you can choose any color). When used in a situation with highlighting hovers like a nav list (which is what we copied for the nav and settings block) they switch between the two colors as appropriate. You can see this in the top nav bar in Mary's screenshot or more relevantly to this dicusssion this example of a nav menu from the bootstrap docs (you might have to scroll down from that anchor to the one titled "Navigation") http://twitter.github.io/bootstrap/base-css.html#icons Moodle's 2.4 action icons are a single color, mid gray. Which works okay against a white or pale gray background but looks a bit odd when put against the blue highlight background, which was the primary motivation for hiding them. It also inspired me to file a bug about converting them to an icon font, see MDL-37231 . Regarding how you line up things when only some have an icon, there was a bug in Bootstrap's github bug tracker about this though I can't Google it up right now. Several people suggested that the words need to line up even if some don't have icons. The Bootstrap guys pointed to Mac OS X menus which always line up against the far left even if some don't have icons. I followed that for the submenu icons. I initally had the submenus so that they didn't exactly line up with the text from the parent branch but stepped in slightly, but it just looked untidy, as if it was supposed to line up but missed. So generally, I tried quite a few different combinations and thought this was the most visually pleasing taking into account a variety of factors and constraints. If you think something else would look or act better then it's relatively easy to mockup using browser tools and share a screenshot. The icons img tags are still there (I do have a renderer that omits them but the code in the nav and settings blocks has off the charts complexity so I've not submitted it) so you can just delete the display: none on the image from the CSS to bring them back, or change it to visibility: hidden to see how it looks with an icon sized empty space where the icon should be. (You might need to set the width to 16px and add padding-right: to 3px or so as well depending on how and what you want to line up with).
          Hide
          Martin Dougiamas added a comment -

          Quite a lot of effort went into those icons in 2.4 ...

          a) I don't really buy the argument about removing the icons from all future themes because they might look odd on some backgrounds. Those themes can remove the icons themselves if they want to.

          b) Would it help this if MDL-37231 was resolved and web fonts were in use so recolouring was trivial?

          Show
          Martin Dougiamas added a comment - Quite a lot of effort went into those icons in 2.4 ... a) I don't really buy the argument about removing the icons from all future themes because they might look odd on some backgrounds. Those themes can remove the icons themselves if they want to. b) Would it help this if MDL-37231 was resolved and web fonts were in use so recolouring was trivial?
          Hide
          David Scotson added a comment -

          Regarding a) I wasn't suggesting they be removed from all future themes, just noting that the fact that they're currently added via img tags rather than applied via CSS (either via simple background images, image sprites or icon fonts) makes them much less dynamic, and therefore clashes with Bootstrap's (and many derivative's/competitor's) basic ideas about what is possible to do with icons these days. They simply don't build their designs around static unchanging icon img tags in the HTML anymore so trying to combine the two concepts doesn't really work. Removing the icons on the other hand was quick and easy and looked visually much better than the alternatives. As I mentioned above, bringing them back in a child theme that doesn't bother itself with Bootstrap's hover effects is one line of CSS. (And I'm not sure how many of the icons in the settings block are even seen by ordinary users rather than admins, which has generally been part of my calculations of what to prioritise.)

          Which isn't to say I didn't spent a fair bit of time and effort trying to get Bootstrap's Glyphicon icon sprite, Bootstrap v3's icon font and Font Awesome's icon font working in those blocks as well as all the other areas of Moodle (You can see the Glyphicon icon sprite in action in the nav/settings blocks if you log in here with user/pass teacher/teacher http://moodle.iyware.com/?theme=bootstrap_renderers though note that's got a theme renderer outputting different HTML for block content). And I thought they all looked pretty great but...

          b) I'm not sure that the direction that bug is going in really addresses the key issues that are blocking the use of an icon font. Even with an ingenious renderer hack that Bas came up with and some dodgy CSS that I contributed we still never got close to being able to replace every icon in Moodle (and a few of the ones we did successfully replace caused weird javascript problems as a result). So that leaves you with half your icons using one (slightly roped together) system and being able to react to hover and change color on a whim to suit your theme etc. and half your icons using the core built in system but being static gray images as a result. You can see the impact in the example image on MDL-37231, the 4 pointed arrow icon hasn't changed to a font icon because it's added via javascript that you can't change from a renderer. You can't see it in the photo but the eye icon now fails to work because the javascript expects an img tag. And there's plenty more like that. As I said in that icon font bug "Converting every bit of Moodle that currently outputs an img icon into the HTML to do something else is the hard bit."

          Show
          David Scotson added a comment - Regarding a) I wasn't suggesting they be removed from all future themes, just noting that the fact that they're currently added via img tags rather than applied via CSS (either via simple background images, image sprites or icon fonts) makes them much less dynamic, and therefore clashes with Bootstrap's (and many derivative's/competitor's) basic ideas about what is possible to do with icons these days. They simply don't build their designs around static unchanging icon img tags in the HTML anymore so trying to combine the two concepts doesn't really work. Removing the icons on the other hand was quick and easy and looked visually much better than the alternatives. As I mentioned above, bringing them back in a child theme that doesn't bother itself with Bootstrap's hover effects is one line of CSS. (And I'm not sure how many of the icons in the settings block are even seen by ordinary users rather than admins, which has generally been part of my calculations of what to prioritise.) Which isn't to say I didn't spent a fair bit of time and effort trying to get Bootstrap's Glyphicon icon sprite, Bootstrap v3's icon font and Font Awesome's icon font working in those blocks as well as all the other areas of Moodle (You can see the Glyphicon icon sprite in action in the nav/settings blocks if you log in here with user/pass teacher/teacher http://moodle.iyware.com/?theme=bootstrap_renderers though note that's got a theme renderer outputting different HTML for block content). And I thought they all looked pretty great but... b) I'm not sure that the direction that bug is going in really addresses the key issues that are blocking the use of an icon font. Even with an ingenious renderer hack that Bas came up with and some dodgy CSS that I contributed we still never got close to being able to replace every icon in Moodle (and a few of the ones we did successfully replace caused weird javascript problems as a result). So that leaves you with half your icons using one (slightly roped together) system and being able to react to hover and change color on a whim to suit your theme etc. and half your icons using the core built in system but being static gray images as a result. You can see the impact in the example image on MDL-37231 , the 4 pointed arrow icon hasn't changed to a font icon because it's added via javascript that you can't change from a renderer. You can't see it in the photo but the eye icon now fails to work because the javascript expects an img tag. And there's plenty more like that. As I said in that icon font bug "Converting every bit of Moodle that currently outputs an img icon into the HTML to do something else is the hard bit."
          Hide
          David Scotson added a comment -

          One potential compromise, use Font Awesome to add an icon font, but only use it in the nav and settings block to start with, and continue to use the current SVG icons elsewhere and do the transition in stages.

          Steps would be:

          1. Add the Font Awesome icon font to replace the Glyphicon icon sprite image stuff in Bootstrap (this is basically how it's designed to be used)

          1a. This (adding fonts to a Moodle theme) requires you to tell the CSS where the font is located within your theme. This can be done in a hacky manner right now, but Petr is working on a way to do it correctly, similar to pix|theme.

          2. You'd need to use Bas's trick of intercepting the pix_icon renderer and replacing the img tag with an appropriate <i> tag and class (something like https://github.com/ds125v/moodle-theme_bootstrap_renderers/blob/master/renderers/bootstrap.php#L159), but you'd be be limited to swapping out icons that are only used in the nav & settings blocks and nowhere else because the pix_icon renderer has no context about where it's being called from. Which might be a short list now that I think about it, but probably includes i/settings and i/navigationitem at least.

          2a. There might be something else you could do here to let you replace more icons, like always (or only when ambiguous) output the i tag followed by the img tag. Then via CSS you could hide img icon tags in the blocks that immediately follow i tags, and vice versa outside. Kind of getting ugly at that point. Or output an empty i tag with a specific class and use the src of the img tag next to it to control the :before content? Or give the auto generated i tags an extra class that you use to hide them outside of the blocks? Or use that same extra class to turn them gray to blend in with the SVGs, but not affect hand coded icons? Oh, I forgot I tried the last one and the SVGs and the icon fonts don't line up together if mix'n'matched.

          3. Once you've got that working you can extend the font by adding Moodle-specfic SVG glyphs to the font, though it's possible you'd not really need to do this until later as there's plenty of overlap on the basic icons.

          As a bonus this could solve the non-appearance of left and right pointing triangle glyphs on Android, and their occasionally varying size on other platforms at the same time, since we could use glyphs.

          Show
          David Scotson added a comment - One potential compromise, use Font Awesome to add an icon font, but only use it in the nav and settings block to start with, and continue to use the current SVG icons elsewhere and do the transition in stages. Steps would be: 1. Add the Font Awesome icon font to replace the Glyphicon icon sprite image stuff in Bootstrap (this is basically how it's designed to be used) 1a. This (adding fonts to a Moodle theme) requires you to tell the CSS where the font is located within your theme. This can be done in a hacky manner right now, but Petr is working on a way to do it correctly, similar to pix|theme. 2. You'd need to use Bas's trick of intercepting the pix_icon renderer and replacing the img tag with an appropriate <i> tag and class (something like https://github.com/ds125v/moodle-theme_bootstrap_renderers/blob/master/renderers/bootstrap.php#L159 ), but you'd be be limited to swapping out icons that are only used in the nav & settings blocks and nowhere else because the pix_icon renderer has no context about where it's being called from. Which might be a short list now that I think about it, but probably includes i/settings and i/navigationitem at least. 2a. There might be something else you could do here to let you replace more icons, like always (or only when ambiguous) output the i tag followed by the img tag. Then via CSS you could hide img icon tags in the blocks that immediately follow i tags, and vice versa outside. Kind of getting ugly at that point. Or output an empty i tag with a specific class and use the src of the img tag next to it to control the :before content? Or give the auto generated i tags an extra class that you use to hide them outside of the blocks? Or use that same extra class to turn them gray to blend in with the SVGs, but not affect hand coded icons? Oh, I forgot I tried the last one and the SVGs and the icon fonts don't line up together if mix'n'matched. 3. Once you've got that working you can extend the font by adding Moodle-specfic SVG glyphs to the font, though it's possible you'd not really need to do this until later as there's plenty of overlap on the basic icons. As a bonus this could solve the non-appearance of left and right pointing triangle glyphs on Android, and their occasionally varying size on other platforms at the same time, since we could use glyphs.
          Hide
          Martin Dougiamas added a comment -

          To add to the pile, here's another idea. Ankit made a proof of concept for modifying the colour of the existing SVG images on the fly: https://github.com/ankitagarwal/moodle/compare/icon That could be controlled by a $THEME->navigationiconcolour or something.

          Otherwise I'm concerned that we have a whole lot of partial solutions here ...

          Show
          Martin Dougiamas added a comment - To add to the pile, here's another idea. Ankit made a proof of concept for modifying the colour of the existing SVG images on the fly: https://github.com/ankitagarwal/moodle/compare/icon That could be controlled by a $THEME->navigationiconcolour or something. Otherwise I'm concerned that we have a whole lot of partial solutions here ...
          Hide
          Frédéric Massart added a comment -

          Icons are also missing in the Navigation block, not only the Administration one.

          Show
          Frédéric Massart added a comment - Icons are also missing in the Navigation block, not only the Administration one.
          Hide
          Frédéric Massart added a comment -

          My +1 to put those icons back in the navigation/administration blocks. I don't think we should get rid of them because they don't fit as is in the layout. From a user experience point of view, they are a great asset as they help to quickly track what link you are looking for in a, already very complex, navigation. Not mentioning that this does not only apply to the administration block, but also in the navigation block. When you expand a course section, you'd usually look for the activity icon to quickly find what activity you're looking for.

          Regarding the grey colour issue. That shouldn't be an issue as any theme can easily override the images, and as the current images are SVG, it makes it even easier to style them. Of course, you would need to create a few version of them if you want :hover effects. Also, if we're trying to follow Bootstrap logic, I don't think that means that we absolutely have to stick with their default colours. It might be much easier to convert the arrow in the navigation to grey and changing the background color, than overriding each of our icons. Isn't that what LESS is for?

          The colour consistency should stay as well, I don't know if that was done on purpose but the icons (from their sprite, or font-color styled) should be consistent with the rest of the core icons. Whatever colour we choose, a lot of effort has been made in 2.4 to create this consistency and we should really work hard in keeping the styles nice and consistent. Or at least have good reasons to change the existing.

          I don't know what should be the solution of the war between SVG and Fonts, but I really think this shouldn't be a priority for now. In any case, the one and only solution for that to take place is to add placeholders <i> everywhere in Moodle, and as you noticed there is a lot of work to do so! We have to define a strong policy about our icons, the way to name the classes, the tags to use, how to use them in JavaScript, etc... Once those placesholders are in the code, each theme can easily opt for fonts, svg, png, sprites, etc...

          Let's not forget about the accessibility issues this represents, an image tag has an alt tag which can be very useful to screenreaders:

          // In current code:
          <a href="delete.php"><img src="delete.png" alt="Delete" class="icon-delete"></i></a>
          // Should become
          <a href="delete.php"><i class="icon-delete"></i><span class="screenreaders-only">Delete</span></a>
          // or (but I'd say that the above is better)
          <a href="delete.php" title="Delete"><i class="icon-delete"></i></a>
          

          That means that the conversation cannot be that simple. And that makes me think that we should do a quick accessibility test on the areas where Simple has define renderers.

          As Martin commented above, Ankit has developed a POC of restyling SVG icons on the fly. I imagine we could have CSS rules such as: background-image: url("delete.svg?#000000") who would return a black delete icon, which could be cached and stored so that it becomes a normal file serving once generated. That would act more or less like fonts, except that it still is one more file for the user to download.

          Show
          Frédéric Massart added a comment - My +1 to put those icons back in the navigation/administration blocks. I don't think we should get rid of them because they don't fit as is in the layout. From a user experience point of view, they are a great asset as they help to quickly track what link you are looking for in a, already very complex, navigation. Not mentioning that this does not only apply to the administration block, but also in the navigation block. When you expand a course section, you'd usually look for the activity icon to quickly find what activity you're looking for. Regarding the grey colour issue. That shouldn't be an issue as any theme can easily override the images, and as the current images are SVG, it makes it even easier to style them. Of course, you would need to create a few version of them if you want :hover effects. Also, if we're trying to follow Bootstrap logic, I don't think that means that we absolutely have to stick with their default colours. It might be much easier to convert the arrow in the navigation to grey and changing the background color, than overriding each of our icons. Isn't that what LESS is for? The colour consistency should stay as well, I don't know if that was done on purpose but the icons (from their sprite, or font-color styled) should be consistent with the rest of the core icons. Whatever colour we choose, a lot of effort has been made in 2.4 to create this consistency and we should really work hard in keeping the styles nice and consistent. Or at least have good reasons to change the existing. I don't know what should be the solution of the war between SVG and Fonts, but I really think this shouldn't be a priority for now. In any case, the one and only solution for that to take place is to add placeholders <i> everywhere in Moodle, and as you noticed there is a lot of work to do so! We have to define a strong policy about our icons, the way to name the classes, the tags to use, how to use them in JavaScript, etc... Once those placesholders are in the code, each theme can easily opt for fonts, svg, png, sprites, etc... Let's not forget about the accessibility issues this represents, an image tag has an alt tag which can be very useful to screenreaders: // In current code: <a href="delete.php"><img src="delete.png" alt="Delete" class="icon-delete"></i></a> // Should become <a href="delete.php"><i class="icon-delete"></i><span class="screenreaders-only">Delete</span></a> // or (but I'd say that the above is better) <a href="delete.php" title="Delete"><i class="icon-delete"></i></a> That means that the conversation cannot be that simple. And that makes me think that we should do a quick accessibility test on the areas where Simple has define renderers. As Martin commented above, Ankit has developed a POC of restyling SVG icons on the fly. I imagine we could have CSS rules such as: background-image: url("delete.svg?#000000") who would return a black delete icon, which could be cached and stored so that it becomes a normal file serving once generated. That would act more or less like fonts, except that it still is one more file for the user to download.
          Hide
          David Scotson added a comment -

          The key issue is that SVGs (or any other kind of image filetype) can't be changed via CSS on :hover if they're linked via an img tag. It's not the SVG file format that's the problem, it's the use of the img tag.

          To do it with CSS you'd need to set them as a background image. Bootstrap 2.3 uses a .png icon sprite in this way, Bootstrap 3 and the popular Font Awesome add-on for Bootstrap 2 uses an icon font which can be altered in the same way you alter normal CSS text via CSS (color, size etc.). Probably the same could be done to swap between different SVG icon background images these days (see http://caniuse.com/svg-css), though given the wider support and greater flexibility of fonts I'm not sure anyone bothers to do it like that.

          Many icons in Moodle, including the ones this bug covers, don't actually need any alt text and currently have empty alt text to signify that, and the one's that do often don't have much context (e.g. you shouldn't really have 50 links on your page that all have the same alt/link/title text of "delete") which is an issue in itself. Also, you shouldn't use title on links for accessibility, they're counter-productive: http://blog.silktide.com/2013/01/i-thought-title-text-improved-accessibility-i-was-wrong/

          As you say, the performance impact of downloading multiple very small files (made worse by having to download them again if you change their color) is a whole other angle on this, and probably the main reason that people started using (the otherwise quite awkward) image sprites and have now shifted to icon fonts.

          Show
          David Scotson added a comment - The key issue is that SVGs (or any other kind of image filetype) can't be changed via CSS on :hover if they're linked via an img tag. It's not the SVG file format that's the problem, it's the use of the img tag. To do it with CSS you'd need to set them as a background image. Bootstrap 2.3 uses a .png icon sprite in this way, Bootstrap 3 and the popular Font Awesome add-on for Bootstrap 2 uses an icon font which can be altered in the same way you alter normal CSS text via CSS (color, size etc.). Probably the same could be done to swap between different SVG icon background images these days (see http://caniuse.com/svg-css ), though given the wider support and greater flexibility of fonts I'm not sure anyone bothers to do it like that. Many icons in Moodle, including the ones this bug covers, don't actually need any alt text and currently have empty alt text to signify that, and the one's that do often don't have much context (e.g. you shouldn't really have 50 links on your page that all have the same alt/link/title text of "delete") which is an issue in itself. Also, you shouldn't use title on links for accessibility, they're counter-productive: http://blog.silktide.com/2013/01/i-thought-title-text-improved-accessibility-i-was-wrong/ As you say, the performance impact of downloading multiple very small files (made worse by having to download them again if you change their color) is a whole other angle on this, and probably the main reason that people started using (the otherwise quite awkward) image sprites and have now shifted to icon fonts.
          Hide
          Frédéric Massart added a comment -

          I understood the key problem, and as I mentioned it will require a lot of work in order to remove the <img> tags everywhere. Though I really agree that this is the solution we're supposed to head towards to. And the accessibility is not a blocking factor, but it just makes the task even more cumbersome.

          In regard with those icons in the navigation block, I think they should be put back in place (recoloured if necessary) as to me they really are great help for the users.

          I don't know about the whole "Bootstrap" history in Moodle, but Moodle is sure not yet ready to go 100% with the bootstrap logic, and so, even if we go progressively, it is important that this takes place transparently to the user, so that it does not affect the user experience. And if required changes affect the user experience, then perhaps raising a "Policy" issue to discuss the outcomes and benefits of such a change is the way to go.

          A good example would be the expandable nodes in the navigation. I personally agree that the current solution is not the right one, but the one provided by Bootstrap does not seem right to me either. Raising an issue where we could discuss and come up with THE solution (and keep history of it for future reference) would probably help a lot.

          Show
          Frédéric Massart added a comment - I understood the key problem, and as I mentioned it will require a lot of work in order to remove the <img> tags everywhere. Though I really agree that this is the solution we're supposed to head towards to. And the accessibility is not a blocking factor, but it just makes the task even more cumbersome. In regard with those icons in the navigation block, I think they should be put back in place (recoloured if necessary) as to me they really are great help for the users. I don't know about the whole "Bootstrap" history in Moodle, but Moodle is sure not yet ready to go 100% with the bootstrap logic, and so, even if we go progressively, it is important that this takes place transparently to the user, so that it does not affect the user experience. And if required changes affect the user experience, then perhaps raising a "Policy" issue to discuss the outcomes and benefits of such a change is the way to go. A good example would be the expandable nodes in the navigation. I personally agree that the current solution is not the right one, but the one provided by Bootstrap does not seem right to me either. Raising an issue where we could discuss and come up with THE solution (and keep history of it for future reference) would probably help a lot.
          Hide
          David Scotson added a comment -

          I've added a screenshot with font_awesome used to add the icons.

          Pushed to a branch here:
          https://github.com/ds125v/moodle/commits/awesome

          This would be my suggested way forward but I'll also push a branch that removes all the Bootstrap stuff from these two blocks. That should both return the standard icons and the ability to navigate via the little expanding triangles.

          Show
          David Scotson added a comment - I've added a screenshot with font_awesome used to add the icons. Pushed to a branch here: https://github.com/ds125v/moodle/commits/awesome This would be my suggested way forward but I'll also push a branch that removes all the Bootstrap stuff from these two blocks. That should both return the standard icons and the ability to navigate via the little expanding triangles.
          Hide
          David Scotson added a comment -

          I've added a screenshot of the nav and settings with most of the Bootstrap styles removed, and pushed it to a branch here:

          https://github.com/ds125v/moodle/commits/standard

          Show
          David Scotson added a comment - I've added a screenshot of the nav and settings with most of the Bootstrap styles removed, and pushed it to a branch here: https://github.com/ds125v/moodle/commits/standard
          Hide
          David Scotson added a comment -

          Attaching new versions of the screenshot, with a selected item, to show icon color change.

          Show
          David Scotson added a comment - Attaching new versions of the screenshot, with a selected item, to show icon color change.
          Hide
          David Scotson added a comment -

          Same view in classic, note the open Question Bank at the bottom.

          This is not currently possible to see in Bootstrap without also changing the page to the Question Bank -> Questions.

          Show
          David Scotson added a comment - Same view in classic, note the open Question Bank at the bottom. This is not currently possible to see in Bootstrap without also changing the page to the Question Bank -> Questions.
          Hide
          Frédéric Massart added a comment -

          Hi David,

          glad to see that it is possible to make those icons back where they were, it looks really nice!

          One thing that is certain when using SVG icons, is that it's very easy to change them. If you want a new icon, you drop a new file and you use it in your CSS file. If you want to replace an image, your proceed the same way. What is the procedure for Font icons?

          • How easy is it to create a new icon using font icons?
          • If I want to edit an icon and replace it with another one, assuming it does not exist in the font file, what should I do?
          • What if I would like to have non-monochrome icons?

          If we're heading towards an easier solution in CSS, we should still keep in mind that it should be easy to replace those icons as well. I am really a newbie when it comes to Font icons, and so I have to admit that I would not be sure of the way to go, but if we were using standard SVGs I would not hesitate a second.

          Though, it comes to my mind that I could update the CSS rules to not use Fonts, but an URL to an image. But is that the solution, isn't that too restrictive and hacky? Isn't that a cumbersome task if I just want to update one single icon? Would browsers behave correctly with a mix of Fonts and SVGs in the same area (let's say navigation)?

          Thanks!
          Fred

          Show
          Frédéric Massart added a comment - Hi David, glad to see that it is possible to make those icons back where they were, it looks really nice! One thing that is certain when using SVG icons, is that it's very easy to change them. If you want a new icon, you drop a new file and you use it in your CSS file. If you want to replace an image, your proceed the same way. What is the procedure for Font icons? How easy is it to create a new icon using font icons? If I want to edit an icon and replace it with another one, assuming it does not exist in the font file, what should I do? What if I would like to have non-monochrome icons? If we're heading towards an easier solution in CSS, we should still keep in mind that it should be easy to replace those icons as well. I am really a newbie when it comes to Font icons, and so I have to admit that I would not be sure of the way to go, but if we were using standard SVGs I would not hesitate a second. Though, it comes to my mind that I could update the CSS rules to not use Fonts, but an URL to an image. But is that the solution, isn't that too restrictive and hacky? Isn't that a cumbersome task if I just want to update one single icon? Would browsers behave correctly with a mix of Fonts and SVGs in the same area (let's say navigation)? Thanks! Fred
          Hide
          Martin Dougiamas added a comment -

          For 2.5 I don't see any option but to restore normal SVG icons ... can we focus on this please?

          Show
          Martin Dougiamas added a comment - For 2.5 I don't see any option but to restore normal SVG icons ... can we focus on this please?
          Hide
          David Scotson added a comment -

          Martin Dougiamas I'll tidy up and submit the "classic" branch above.

          Frédéric Massart To customize the icons you just need the custom SVG and one of the many free tools for building icon fonts e.g http://icomoon.io/app/ though that's assuming you can't find something to use in the many free collections. One of the purposes of icons is to be easily recognizable after all. For multi-colors there are hacky workarounds for non-monochrome fonts (basically layering different colored monochrome icons), and Mozilla and others are pushing for full color SVG glyphs in fonts but that's for the future. Today you'd probably be best applying SVG via CSS as a background image (I believe Moodle even has support for falling back to PNGs in this case). All the current benefits of fonts are pretty compelling for many uses though.

          Show
          David Scotson added a comment - Martin Dougiamas I'll tidy up and submit the "classic" branch above. Frédéric Massart To customize the icons you just need the custom SVG and one of the many free tools for building icon fonts e.g http://icomoon.io/app/ though that's assuming you can't find something to use in the many free collections. One of the purposes of icons is to be easily recognizable after all. For multi-colors there are hacky workarounds for non-monochrome fonts (basically layering different colored monochrome icons), and Mozilla and others are pushing for full color SVG glyphs in fonts but that's for the future. Today you'd probably be best applying SVG via CSS as a background image (I believe Moodle even has support for falling back to PNGs in this case). All the current benefits of fonts are pretty compelling for many uses though.
          Hide
          Frédéric Massart added a comment -

          Thanks David,

          this looks good to me. Here are some really minor issues:

          Feel free to push for integration whenever ready!

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Thanks David, this looks good to me. Here are some really minor issues: theme/bootstrap/less/moodle/blocks.less:18 theme/simple/config.php:49 I don't know if there is any incidence, but there shouldn't be any , at the end of those lines. you could mention the component (themes) in the commit message: http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages Feel free to push for integration whenever ready! Cheers, Fred
          Hide
          David Scotson added a comment -

          Thanks Frédéric,

          I'll remove that extra comma and add the component.

          I'm fairly sure that another reviewer suggested that PHP array's should have the comma at the end (it makes diffs cleaner since if you add or remove a line then you only need to change that line). Any idea where I could find the official policy?

          I don't actually know how to "push for integration", any hints?

          Show
          David Scotson added a comment - Thanks Frédéric, I'll remove that extra comma and add the component. I'm fairly sure that another reviewer suggested that PHP array's should have the comma at the end (it makes diffs cleaner since if you add or remove a line then you only need to change that line). Any idea where I could find the official policy? I don't actually know how to "push for integration", any hints?
          Hide
          Mary Evans added a comment - - edited

          The correct way for Commit message in themes is add the component name just after the issue MDL-xxxxx as follows using your current branch here as an example:

          MDL-39138 theme_bootstrap: Revert changes to Settings/Nav blocks

          Also you should see "Submit for Integration" at the same place you see "Workflow" and "Request Peer Review".

          If you don't see these then you don't have permissions set which need to be attended to.

          Show
          Mary Evans added a comment - - edited The correct way for Commit message in themes is add the component name just after the issue MDL-xxxxx as follows using your current branch here as an example: MDL-39138 theme_bootstrap: Revert changes to Settings/Nav blocks Also you should see "Submit for Integration" at the same place you see "Workflow" and "Request Peer Review". If you don't see these then you don't have permissions set which need to be attended to.
          Hide
          Mary Evans added a comment -

          The comma in an array shouldn't be there if it's written inline. However, according to Sam Hemelryk, it's OK when written in a list. I try to not use them, but the all the php code in every theme is peppered with them. They started in Moodle 2.0 and since most things get copied...we carry on with bad practice.

          Show
          Mary Evans added a comment - The comma in an array shouldn't be there if it's written inline. However, according to Sam Hemelryk, it's OK when written in a list. I try to not use them, but the all the php code in every theme is peppered with them. They started in Moodle 2.0 and since most things get copied...we carry on with bad practice.
          Hide
          David Scotson added a comment -

          I've fixed the stray comma in the LESS, left the comma in the PHP, and added theme_bootstrap to the commit message, merged the commits and pushed it again.

          I don't see any "submit for integration" button, just the peer review and continue development.

          Show
          David Scotson added a comment - I've fixed the stray comma in the LESS, left the comma in the PHP, and added theme_bootstrap to the commit message, merged the commits and pushed it again. I don't see any "submit for integration" button, just the peer review and continue development.
          Hide
          Mary Evans added a comment -

          I set it for Integration Review, only problem is that when Moodle is updated tonight or tomorrow, you will need to rebase this branch against your updated master branch. You will get notification to do this as and when Moodle is updated.

          Show
          Mary Evans added a comment - I set it for Integration Review, only problem is that when Moodle is updated tonight or tomorrow, you will need to rebase this branch against your updated master branch. You will get notification to do this as and when Moodle is updated.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          (I've recompiled the generated.css file here coz it was conflict-merging)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (I've recompiled the generated.css file here coz it was conflict-merging)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing this, visual comparison with standard reveals all the icons in navigation & settings blocks to be the same.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing this, visual comparison with standard reveals all the icons in navigation & settings blocks to be the same.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: