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

In Base theme, add a clearfix class so that the body isn't 0 (zero) height

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Turn theme designer mode on
      2. Select Base theme in normal way using theme selector
      3. Using 'Firebug' right click the main body of the page and choose 'Inspect Element', then go to the layout tab and ensure that the height is something greater than 0.
      Show
      Turn theme designer mode on Select Base theme in normal way using theme selector Using 'Firebug' right click the main body of the page and choose 'Inspect Element', then go to the layout tab and ensure that the height is something greater than 0.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31169_master

      Description

      In Moodle 2 base theme the body is 0 height due to floated contents. The solution is to add a clearfix div just before the closing body div. However, this is an annoyance when working with the base theme and could be fixed.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that.
            Hide
            lazydaisy Mary Evans added a comment -

            I reported this ages ago in the forum and no one took any notice, so I thought I was in the wrong!

            I wish now I had gone with my instincts and fixed this before now.

            I'll do it now.
            Thanks!

            Show
            lazydaisy Mary Evans added a comment - I reported this ages ago in the forum and no one took any notice, so I thought I was in the wrong! I wish now I had gone with my instincts and fixed this before now. I'll do it now. Thanks!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Mary,

            I've been looking at this for quite a while but too be truthful I'm not sure of it necessity.
            What is adding this empty div solving exactly?
            If there is a reason to put it in all good and well, but at present I don't see what it offers that isn't already achievable. Especially given this is overridden in the base layouts which 90% of theme's appear to override anyway.
            I've looked to the linked issue but I can't see how the solution to that lies in ensuring the body tag full encompases the page content.
            In regards to page flow, the breakage is intentional as we wanted to provide the 2,1,3 layout.

            I'll wait for your reply before acting further upon this issue.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Mary, I've been looking at this for quite a while but too be truthful I'm not sure of it necessity. What is adding this empty div solving exactly? If there is a reason to put it in all good and well, but at present I don't see what it offers that isn't already achievable. Especially given this is overridden in the base layouts which 90% of theme's appear to override anyway. I've looked to the linked issue but I can't see how the solution to that lies in ensuring the body tag full encompases the page content. In regards to page flow, the breakage is intentional as we wanted to provide the 2,1,3 layout. I'll wait for your reply before acting further upon this issue. Cheers Sam
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Try adding...

            #page {border: 5px solid red;}

            and then tell me why it has a 10px solid line at the top of the page when you are telling it to put a border around the perimeter of the page?

            You once told me that you can cure it by adding clearfix before the last div. It's in the themes forum discussions somewhere in 2010.

            I found the discussion...but as it was 2 years ago I got my facts mixed up
            http://moodle.org/mod/forum/discuss.php?d=152667 but I still think there is a problem.

            This lack of '#page flow', as I call it, in Base theme has always been a source of annoyance. So too is the fact 'navbar' is inside page-header, whereas in Moodle 1.9 it was always outside and easier to style.

            Please allow this 'clearfix' to be added, and if possible, give me the OK to fix the navbar too and then, as far as I am concerned, it will make creating new themes easier, as we will have a good layout, which we know works.

            Mary

            Show
            lazydaisy Mary Evans added a comment - - edited Try adding... #page {border: 5px solid red;} and then tell me why it has a 10px solid line at the top of the page when you are telling it to put a border around the perimeter of the page? You once told me that you can cure it by adding clearfix before the last div. It's in the themes forum discussions somewhere in 2010. I found the discussion...but as it was 2 years ago I got my facts mixed up http://moodle.org/mod/forum/discuss.php?d=152667 but I still think there is a problem. This lack of '#page flow', as I call it, in Base theme has always been a source of annoyance. So too is the fact 'navbar' is inside page-header, whereas in Moodle 1.9 it was always outside and easier to style. Please allow this 'clearfix' to be added, and if possible, give me the OK to fix the navbar too and then, as far as I am concerned, it will make creating new themes easier, as we will have a good layout, which we know works. Mary
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hehe for a bit of fun try adding a 5px solid red border to the div element with the clear fix.

            Certainly if it can be achieved by adding clearfix to the page object I would be a much bigger fan of that as it's not introducing an empty div.
            Either way I'll discuss this with Eloy, if he sees no problem then it can go in.
            In regards to moving the navbar that is a pretty serious change, my biggest concern with it would be backwards compatibility. Will themes that have based themselves on another theme and re-styled the navbar break. Of course we can easily change core theme's however as 99% of theme's inevitably inherit from base I think any significant move would cause mass breakage outside of core.

            I'm not against it, I don't have any preference to it being in the header of not but certainly I don't think it is worth breaking all theme's in order to move it. If people want it moved they can currently move it in their own theme's.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hehe for a bit of fun try adding a 5px solid red border to the div element with the clear fix. Certainly if it can be achieved by adding clearfix to the page object I would be a much bigger fan of that as it's not introducing an empty div. Either way I'll discuss this with Eloy, if he sees no problem then it can go in. In regards to moving the navbar that is a pretty serious change, my biggest concern with it would be backwards compatibility. Will themes that have based themselves on another theme and re-styled the navbar break. Of course we can easily change core theme's however as 99% of theme's inevitably inherit from base I think any significant move would cause mass breakage outside of core. I'm not against it, I don't have any preference to it being in the header of not but certainly I don't think it is worth breaking all theme's in order to move it. If people want it moved they can currently move it in their own theme's. Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Mary,

            I've had a big discussion with Eloy about this issue this morning and decided to integrate it but on master only.
            The change is for the better, however master only because there is a chance that it could inadvertently cause problems for the likes of scorm and lti as they use iframes and/or JS measuring the size of the window and specific divs.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Mary, I've had a big discussion with Eloy about this issue this morning and decided to integrate it but on master only. The change is for the better, however master only because there is a chance that it could inadvertently cause problems for the likes of scorm and lti as they use iframes and/or JS measuring the size of the window and specific divs. Cheers Sam
            Hide
            lazydaisy Mary Evans added a comment -

            That's fine by me.
            Thanks

            Show
            lazydaisy Mary Evans added a comment - That's fine by me. Thanks
            Hide
            poltawski Dan Poltawski added a comment -

            I tested this by looking for problems in themes and also using the debug console to add a border to display the change like Mary suggested.

            Looks good

            Show
            poltawski Dan Poltawski added a comment - I tested this by looking for problems in themes and also using the debug console to add a border to display the change like Mary suggested. Looks good
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FCT (fixed, closing, thanks). Ciao

            "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
            ~ Benjamin Disraeli

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12