Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5, 2.6
    • Fix Version/s: 2.5.1
    • Component/s: Themes
    • Labels:
    • Rank:
      50817

      Description

      The 'Turn editing on' button is misaligned on the navigation bar.

      In 'moodle/modules.less':

      .breadcrumb-button {
          float: right;
          margin-top: 3px;
      }
      

      needs to be:

      .breadcrumb-button {
          float: right;
          margin-top: 4px;
      }
      

        Activity

        Hide
        Mary Evans added a comment -

        I would be inclined to add 2px at the top and 5px on the right and add nothing to the bottom and left as these are not needed. So would be:

        .singlebutton div {
            display: inline-block;
            margin: 2px 5px 0 0;
        }
        
        Show
        Mary Evans added a comment - I would be inclined to add 2px at the top and 5px on the right and add nothing to the bottom and left as these are not needed. So would be: .singlebutton div { display: inline-block; margin: 2px 5px 0 0; }
        Hide
        Mary Evans added a comment -

        You may need to check RTL CSS for this button if it exists?

        Show
        Mary Evans added a comment - You may need to check RTL CSS for this button if it exists?
        Hide
        David Scotson added a comment -

        I think .singlebutton div will hit a lot more places than just this. The .singlebutton class is messed up because it's also used (drumroll please) for multiple buttons side by side, which makes it tricky to style correctly without visually checking basically every hand built form througout Moodle. That's one of the things I'd like to see fixed in the HTML this dev cycle.

        The button is actually placed over the navbar by floating it over the top of the navbar then lining it up with margins. I'd suggest targetting that code for a fix and changing the margins on .breadcrumb-button which is in modules.less (which is the kind of catch all file for bits with no where else to go). It also has a RTL equivalent you'll need to update if you take Mary's advice and nudge it in toward the center a bit. I just tried that and I think it looks very slightly better, but then you're taking up vital pixels that'll be needed on phone size devices. (That of course can be fixed with responsiveness, but that adds complexity).

        Show
        David Scotson added a comment - I think .singlebutton div will hit a lot more places than just this. The .singlebutton class is messed up because it's also used (drumroll please) for multiple buttons side by side, which makes it tricky to style correctly without visually checking basically every hand built form througout Moodle. That's one of the things I'd like to see fixed in the HTML this dev cycle. The button is actually placed over the navbar by floating it over the top of the navbar then lining it up with margins. I'd suggest targetting that code for a fix and changing the margins on .breadcrumb-button which is in modules.less (which is the kind of catch all file for bits with no where else to go). It also has a RTL equivalent you'll need to update if you take Mary's advice and nudge it in toward the center a bit. I just tried that and I think it looks very slightly better, but then you're taking up vital pixels that'll be needed on phone size devices. (That of course can be fixed with responsiveness, but that adds complexity).
        Hide
        Gareth J Barnard added a comment - - edited

        Dear Mary Evans and David Scotson,

        Thanks I'll look into a fix in 'modules.less'. As this is a vertical fix then RTL does not come into it. Looking at http://www.w3schools.com/css/css_margin.asp and my original suggested fix of:

        margin: 1px 5px 5px;
        

        where the left and right margins were the same, then there would be no need for a separate RTL selector attribute.

        As for taking up vital pixels on phone devices, pragmatically 1px is not going to break the bank at Monty Carlo especially as I'm only slightly moving a button within an existing area which does not affect it's size .

        Cheers,

        Gareth

        P.S. Just thought it was odd that '.singlebutton div' came up as the applied selector instead of '.breadcrumb-button' when I used FireLess to find the CSS that manipulated the button - humm.

        Show
        Gareth J Barnard added a comment - - edited Dear Mary Evans and David Scotson , Thanks I'll look into a fix in 'modules.less'. As this is a vertical fix then RTL does not come into it. Looking at http://www.w3schools.com/css/css_margin.asp and my original suggested fix of: margin: 1px 5px 5px; where the left and right margins were the same, then there would be no need for a separate RTL selector attribute. As for taking up vital pixels on phone devices, pragmatically 1px is not going to break the bank at Monty Carlo especially as I'm only slightly moving a button within an existing area which does not affect it's size . Cheers, Gareth P.S. Just thought it was odd that '.singlebutton div' came up as the applied selector instead of '.breadcrumb-button' when I used FireLess to find the CSS that manipulated the button - humm.
        Hide
        David Scotson added a comment -

        Ah, I actually misread Mary's comment of adding further margin on the right, when she was actually suggesting taking it away from the left and bottom. (Again, I'd not recommend that because of the effect would have on all the other .singlebutton divs, which are pretty precarious as it is.)

        So, yes in that case no RTL fix needed and no extra width taken up if you make your vertical change in module.less.

        Show
        David Scotson added a comment - Ah, I actually misread Mary's comment of adding further margin on the right, when she was actually suggesting taking it away from the left and bottom. (Again, I'd not recommend that because of the effect would have on all the other .singlebutton divs, which are pretty precarious as it is.) So, yes in that case no RTL fix needed and no extra width taken up if you make your vertical change in module.less.
        Hide
        Mary Evans added a comment -

        @Gareth, I should have realised that it would be styled elsewhere for general useage.

        @David, +1

        Show
        Mary Evans added a comment - @Gareth, I should have realised that it would be styled elsewhere for general useage. @David, +1
        Hide
        Gareth J Barnard added a comment -

        Dear Mary Evans and David Scotson,

        Thank you both - so much still to learn about themes and Bootstrap.

        Cheers,

        Gareth

        Show
        Gareth J Barnard added a comment - Dear Mary Evans and David Scotson , Thank you both - so much still to learn about themes and Bootstrap. Cheers, Gareth
        Hide
        Gareth J Barnard added a comment -

        Ok,

        This is driving me nuts! If I change the margin to '4px' instead of '3px' then it works but means that the between the top and the button is a '4px' difference but between the bottom of the button and the bottom of the navigation bar is '5px'. Measuring the button turns out at '27px' and then comparing the size of the button on the 'Search Forums' block with a 'Go' button it is obvious that the button is '1px' to small when aligned with the search box. So on this global premise then, change the padding of the button from 'padding: 4px 12px' to 'padding: 5px 12px 4px' using http://www.w3schools.com/cssref/pr_padding.asp as a guide. But! It turns out the definition '.btn' is a Bootstrap 'less' file so no go - unless we agree to alter. So, reduce the size of the navigation bar, but wait! The text is correctly positioned at 13px top and bottom (ignoring the shadow) - so umm no.

        So, what I propose is the '3px' to '4px' margin change so that the button is not so 'in your face' and we could look at reducing text boxes by '1px'.

        I know that this might all seem small and trivial, but I think it's worth doing to get it right. Lots of small changes add up to one good overall effect.

        Cheers,

        Gareth

        Show
        Gareth J Barnard added a comment - Ok, This is driving me nuts! If I change the margin to '4px' instead of '3px' then it works but means that the between the top and the button is a '4px' difference but between the bottom of the button and the bottom of the navigation bar is '5px'. Measuring the button turns out at '27px' and then comparing the size of the button on the 'Search Forums' block with a 'Go' button it is obvious that the button is '1px' to small when aligned with the search box. So on this global premise then, change the padding of the button from 'padding: 4px 12px' to 'padding: 5px 12px 4px' using http://www.w3schools.com/cssref/pr_padding.asp as a guide. But! It turns out the definition '.btn' is a Bootstrap 'less' file so no go - unless we agree to alter. So, reduce the size of the navigation bar, but wait! The text is correctly positioned at 13px top and bottom (ignoring the shadow) - so umm no. So, what I propose is the '3px' to '4px' margin change so that the button is not so 'in your face' and we could look at reducing text boxes by '1px'. I know that this might all seem small and trivial, but I think it's worth doing to get it right. Lots of small changes add up to one good overall effect. Cheers, Gareth
        Hide
        Mary Evans added a comment - - edited

        @Gareth, you will find that when styling something specific, you need to look at alternative CSS mark-up, in other words assume it has nothing and you are starting from scratch.

        So

        .nav.breadcrumb-button { margin: 0; padding: 0} /* has no styles */
        
        nav.breadcrumb-button .singlebutton { /* so the singlebutton container needs to be */
            height: 27px; /* same height as button */
            padding: 5px 0 4px; /* padding top/bottom to make it sit in the center */
        }
        

        We know that the button itself (input/submit) is styled with this...

        button, input.form-submit, input[type="button"], input[type="submit"], input[type="reset"] {
            -moz-border-bottom-colors: none;
            -moz-border-left-colors: none;
            -moz-border-right-colors: none;
            -moz-border-top-colors: none;
            background-color: #F5F5F5;
            background-image: linear-gradient(to bottom, #FFFFFF, #E6E6E6);
            background-repeat: repeat-x;
            border-color: rgba(0, 0, 0, 0.1) rgba(0, 0, 0, 0.1) #B3B3B3;
            border-image: none;
            border-radius: 4px 4px 4px 4px;
            border-style: solid;
            border-width: 1px;
            box-shadow: 0 1px 0 rgba(255, 255, 255, 0.2) inset, 0 1px 2px rgba(0, 0, 0, 0.05);
            color: #333333;
            cursor: pointer;
            display: inline-block;
            font-size: 14px;
            line-height: 20px;
            margin-bottom: 0;
            padding: 4px 12px;
            text-align: center;
            text-shadow: 0 1px 1px rgba(255, 255, 255, 0.75);
            vertical-align: middle;
        }
        

        So no need to touch that. But the only thing left is...

        nav.breadcrumb-button .singlebutton div {
            margin: 0 5px; /* which adds left/right margins but not top/botton margins as we have positioned the button where we want it using the .singlebutton container */
        }
        

        I must admit I tend to use position: relative to fix the navbar button, and other elements on a page.

        Show
        Mary Evans added a comment - - edited @Gareth, you will find that when styling something specific, you need to look at alternative CSS mark-up, in other words assume it has nothing and you are starting from scratch. So .nav.breadcrumb-button { margin: 0; padding: 0} /* has no styles */ nav.breadcrumb-button .singlebutton { /* so the singlebutton container needs to be */ height: 27px; /* same height as button */ padding: 5px 0 4px; /* padding top/bottom to make it sit in the center */ } We know that the button itself (input/submit) is styled with this... button, input.form-submit, input[type= "button" ], input[type= "submit" ], input[type= "reset" ] { -moz-border-bottom-colors: none; -moz-border-left-colors: none; -moz-border-right-colors: none; -moz-border-top-colors: none; background-color: #F5F5F5; background-image: linear-gradient(to bottom, #FFFFFF, #E6E6E6); background-repeat: repeat-x; border-color: rgba(0, 0, 0, 0.1) rgba(0, 0, 0, 0.1) #B3B3B3; border-image: none; border-radius: 4px 4px 4px 4px; border-style: solid; border-width: 1px; box-shadow: 0 1px 0 rgba(255, 255, 255, 0.2) inset, 0 1px 2px rgba(0, 0, 0, 0.05); color: #333333; cursor: pointer; display: inline-block; font-size: 14px; line-height: 20px; margin-bottom: 0; padding: 4px 12px; text-align: center; text-shadow: 0 1px 1px rgba(255, 255, 255, 0.75); vertical-align: middle; } So no need to touch that. But the only thing left is... nav.breadcrumb-button .singlebutton div { margin: 0 5px; /* which adds left/right margins but not top/botton margins as we have positioned the button where we want it using the .singlebutton container */ } I must admit I tend to use position: relative to fix the navbar button, and other elements on a page.
        Hide
        Mary Evans added a comment -

        That's not to say that the above is the way to do it...it's just another way.
        In actual fact I don't know why so many emty divs are added to Moodle. It is so very easy to mess the layout up when divs are not closed.

        Anyway +1 for your fix I'll push it for integration.
        Cheers

        Show
        Mary Evans added a comment - That's not to say that the above is the way to do it...it's just another way. In actual fact I don't know why so many emty divs are added to Moodle. It is so very easy to mess the layout up when divs are not closed. Anyway +1 for your fix I'll push it for integration. Cheers
        Hide
        Gareth J Barnard added a comment -

        Dear Mary Evans,

        Thank you - sorry for not replying sooner - so much to take in

        I had not realised that '0' was an indication of an inherited default value -> http://www.w3schools.com/cssref/pr_margin.asp

        I tend to take what is there and experiment in FireBug to see what works.

        I think that a lot has to be said for Damyon's draft framework > http://docs.moodle.org/dev/User:Damyon_Wiese/Draft_CSS_Framework < so hopefully we will be able to work towards seeing the wood for the trees.

        I think that the height of '.singlebutton div' needs to be '28px' - but that's another issue!

        Looking at your example, where is the 'margin-top: 4px;' implemented as you have 'margin: 0 5px;' and inherited 'margin-bottom: 0;' & 'padding: 4px 12px;'?

        Cheers,

        Gareth

        Show
        Gareth J Barnard added a comment - Dear Mary Evans , Thank you - sorry for not replying sooner - so much to take in I had not realised that '0' was an indication of an inherited default value -> http://www.w3schools.com/cssref/pr_margin.asp I tend to take what is there and experiment in FireBug to see what works. I think that a lot has to be said for Damyon's draft framework > http://docs.moodle.org/dev/User:Damyon_Wiese/Draft_CSS_Framework < so hopefully we will be able to work towards seeing the wood for the trees. I think that the height of '.singlebutton div' needs to be '28px' - but that's another issue! Looking at your example, where is the 'margin-top: 4px;' implemented as you have 'margin: 0 5px;' and inherited 'margin-bottom: 0;' & 'padding: 4px 12px;'? Cheers, Gareth
        Hide
        Gareth J Barnard added a comment -

        P.S. I'm having a look at MDL-39798 and see your point about empty 'div's - pointless!

        Show
        Gareth J Barnard added a comment - P.S. I'm having a look at MDL-39798 and see your point about empty 'div's - pointless!
        Hide
        Mary Evans added a comment -

        The problem with CSS is that you can end up in in a mess if the cascade is out of sequence. Not sure how LESS copes with that.

        Show
        Mary Evans added a comment - The problem with CSS is that you can end up in in a mess if the cascade is out of sequence. Not sure how LESS copes with that.
        Hide
        Gareth J Barnard added a comment -

        I don't think it does.

        Show
        Gareth J Barnard added a comment - I don't think it does.
        Hide
        Dan Poltawski added a comment -

        Integrated to master and 25, thanks Gareth!

        (and Mary/David for contributing to this issue, impressive amount of discussion for this pixel! )

        Show
        Dan Poltawski added a comment - Integrated to master and 25, thanks Gareth! (and Mary/David for contributing to this issue, impressive amount of discussion for this pixel! )
        Hide
        Adrian Greeve added a comment -

        Tested on 2.5 and master integration
        The turn editing on button looks like it's aligned right in the middle.
        Test passed.

        Show
        Adrian Greeve added a comment - Tested on 2.5 and master integration The turn editing on button looks like it's aligned right in the middle. Test passed.
        Hide
        Marina Glancy added a comment -

        Thanks for your awesome work! This has now become a part of Moodle.

        Closing as fixed!

        Show
        Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: