Details

    • Rank:
      49756

      Description

      OK, so this is not only an issue in bootstrap but it's certainly the place to fix it.

      The problem is that the left-side of the forms (with the labels) is incredibly small and doesn't expand on large screens. This makes the labels wrap and look pretty bad (screenshot attached).

      I should note that Bootstrap does a great thing with forms on small screens by wrapping form elements under the labels, but to be truly responsive we should also look awesome on larger screens.

        Issue Links

          Activity

          Hide
          David Scotson added a comment -

          The current width of the labels is the Bootstrap default of 160px.

          It shouldn't be too hard to bump this up at larger screen sizes via the usual responsive stuff. I generally bump this up to 180px even for the medium sized screens as Moodle has some pretty long labels. That latter fix is just a case of changing a variable (@horizontalComponentOffset) and recompiling. Maybe that second change is enough (it actually probably depends on the end user language to some degree as well).

          Show
          David Scotson added a comment - The current width of the labels is the Bootstrap default of 160px. It shouldn't be too hard to bump this up at larger screen sizes via the usual responsive stuff. I generally bump this up to 180px even for the medium sized screens as Moodle has some pretty long labels. That latter fix is just a case of changing a variable (@horizontalComponentOffset) and recompiling. Maybe that second change is enough (it actually probably depends on the end user language to some degree as well).
          Hide
          Martin Dougiamas added a comment -

          Jason can you try this out and produce a patch here?

          Show
          Martin Dougiamas added a comment - Jason can you try this out and produce a patch here?
          Hide
          Jason Fowler added a comment -

          Will do Martin.

          Show
          Jason Fowler added a comment - Will do Martin.
          Hide
          Jason Fowler added a comment -

          Two new sizes, depending on the screen size, 180px @ 768 screen width, and 200px @ anything over 980px.

          Show
          Jason Fowler added a comment - Two new sizes, depending on the screen size, 180px @ 768 screen width, and 200px @ anything over 980px.
          Hide
          David Scotson added a comment - - edited

          The linked patch only changes the labels in .mform. There's other types of forms, and there's other stuff even within .mforms that needs to shift to compensate/match the wider labels.

          Setting the @horizontalComponentOffset as 180px where this link points, after these other Moodle specific variables should take care of the 160->180px change with one line:

          https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle.less#L17-L19

          For the wider screens you'd need to examine the compiled output CSS (easier to do if it's not compressed) and locate the section generated from this file:

          https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle/forms.less

          Any lines in the output setting something (padding,margin,width) to 180px or 200px (or 160 and 180 in the default setup) probably needs to be cut and pasted into the responsive stuff and then updated to whatever the new values are to override them.

          edit: just to explain those numbers more, the gap between the labels and the controls is 20px, so the Bootstrap controls have a left-margin of X + 20px. Getting Moodle's forms to match sometimes involves fiddling with padding instead since the HTML is different.

          Show
          David Scotson added a comment - - edited The linked patch only changes the labels in .mform. There's other types of forms, and there's other stuff even within .mforms that needs to shift to compensate/match the wider labels. Setting the @horizontalComponentOffset as 180px where this link points, after these other Moodle specific variables should take care of the 160->180px change with one line: https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle.less#L17-L19 For the wider screens you'd need to examine the compiled output CSS (easier to do if it's not compressed) and locate the section generated from this file: https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle/forms.less Any lines in the output setting something (padding,margin,width) to 180px or 200px (or 160 and 180 in the default setup) probably needs to be cut and pasted into the responsive stuff and then updated to whatever the new values are to override them. edit: just to explain those numbers more, the gap between the labels and the controls is 20px, so the Bootstrap controls have a left-margin of X + 20px. Getting Moodle's forms to match sometimes involves fiddling with padding instead since the HTML is different.
          Hide
          David Scotson added a comment -

          Minor correction: the default value of @horizontalComponentOffset is already 180px. It calculates the width of the label by subtracting 20px from that variable, rather than adding 20px to it for the offset.

          So we'd want:

          @horizontalComponentOffset: 200px;

          to increase the labels by 20px from what they are now.

          Show
          David Scotson added a comment - Minor correction: the default value of @horizontalComponentOffset is already 180px. It calculates the width of the label by subtracting 20px from that variable, rather than adding 20px to it for the offset. So we'd want: @horizontalComponentOffset: 200px; to increase the labels by 20px from what they are now.
          Hide
          Jason Fowler added a comment -

          I'm a little confused. So I would remove what I have done, and add

          @media (min-width: 980px) {
          @horizontalComponentOffset: 220px;
          }
          
          @media (min-width: 768px) and (max-width: 979px) {
          @horizontalComponentOffset: 200px;
          }
          

          at

          https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle.less#L17-L19

          Show
          Jason Fowler added a comment - I'm a little confused. So I would remove what I have done, and add @media (min-width: 980px) { @horizontalComponentOffset: 220px; } @media (min-width: 768px) and (max-width: 979px) { @horizontalComponentOffset: 200px; } at https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle.less#L17-L19
          Hide
          Jason Fowler added a comment -

          no, that won't work. Need to create may need to create

          @horizontalComponentOffset768: 200px;
          @horizontalComponentOffset1200: 220px;

          and implement them some how.

          Show
          Jason Fowler added a comment - no, that won't work. Need to create may need to create @horizontalComponentOffset768: 200px; @horizontalComponentOffset1200: 220px; and implement them some how.
          Hide
          David Scotson added a comment -

          I can see where you're going with that, but it doesn't work like that (would be handy if it did though).

          The @variable stuff is part of LESS and all gets done at compile time on the dev's machine, where it gets replaced with static values like 180px in the output CSS.

          The @media (min-width: 980px) stuff is part of CSS and gets looked at when the users browser is deciding how to style a page. You probably don't want to be setting LESS variables here, since setting a variable has no visible output in the CSS, you need to use the variable in a CSS rule in order to create some CSS.

          Generally with the CSS3 @media stuff you have a default value that applies to everything, then you specialize/change it for certain sizes.

          We want to first change the default width that applies to everything, and for that you need to add just the single line "@horizontalComponentOffset: 220px;" next to the other variable assignments here:

          https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle.less#L17-L19

          As that gets compiled the Bootstrap LESS does various calculations based on that value and then uses the resulting calculated values in somewhere between 5 and 10 different places in the outputted CSS.

          If you then want to change how forms are laid out when they respond to wider screen widths you need to supply the larger value for each of times that sizes are specified in the output CSS.

          I'll hopefully get a chance to make a patch for the latter task later tonight. It touches on some more advanced parts of CSS, LESS and Bootstrap all at the same time so it's probably easier to demonstrate than describe.

          Show
          David Scotson added a comment - I can see where you're going with that, but it doesn't work like that (would be handy if it did though). The @variable stuff is part of LESS and all gets done at compile time on the dev's machine, where it gets replaced with static values like 180px in the output CSS. The @media (min-width: 980px) stuff is part of CSS and gets looked at when the users browser is deciding how to style a page. You probably don't want to be setting LESS variables here, since setting a variable has no visible output in the CSS, you need to use the variable in a CSS rule in order to create some CSS. Generally with the CSS3 @media stuff you have a default value that applies to everything, then you specialize/change it for certain sizes. We want to first change the default width that applies to everything, and for that you need to add just the single line "@horizontalComponentOffset: 220px;" next to the other variable assignments here: https://github.com/phalacee/moodle/blob/14499fc9f07b075a1516fa84cf069f5a87def8a7/theme/bootstrap/less/moodle.less#L17-L19 As that gets compiled the Bootstrap LESS does various calculations based on that value and then uses the resulting calculated values in somewhere between 5 and 10 different places in the outputted CSS. If you then want to change how forms are laid out when they respond to wider screen widths you need to supply the larger value for each of times that sizes are specified in the output CSS. I'll hopefully get a chance to make a patch for the latter task later tonight. It touches on some more advanced parts of CSS, LESS and Bootstrap all at the same time so it's probably easier to demonstrate than describe.
          Hide
          David Scotson added a comment -

          I've done the first bit:

          https://github.com/ds125v/moodle/compare/master...wip-MDL-39158-master

          but I need to leave the office now, I might be able to do the other bit later.

          Show
          David Scotson added a comment - I've done the first bit: https://github.com/ds125v/moodle/compare/master...wip-MDL-39158-master but I need to leave the office now, I might be able to do the other bit later.
          Hide
          David Scotson added a comment -

          I've added a second commit to the above that adds the wider setting.

          I've not tested it as I'm on a netbook, and so don't have the screen width.

          If anyone wants to take it from here and do the testing and decide on what the exact values should be then that's fine by me.

          Show
          David Scotson added a comment - I've added a second commit to the above that adds the wider setting. I've not tested it as I'm on a netbook, and so don't have the screen width. If anyone wants to take it from here and do the testing and decide on what the exact values should be then that's fine by me.
          Hide
          Jason Fowler added a comment - - edited

          will test your branch later today, and if it works correctly, I will push it for integration.

          Show
          Jason Fowler added a comment - - edited will test your branch later today, and if it works correctly, I will push it for integration.
          Hide
          Jason Fowler added a comment -

          Sorry I haven't gotten to this yet, I've been caught up with computer issues and testing. Will get to it ASAP.

          Show
          Jason Fowler added a comment - Sorry I haven't gotten to this yet, I've been caught up with computer issues and testing. Will get to it ASAP.
          Hide
          Jason Fowler added a comment -

          Works fine from what I tested David. And the code looks great. It's almost exactly what I had written before you posted this. Only I had more variations. I don't think the extras I came up with are really required though. Will push yours for integration now.

          Show
          Jason Fowler added a comment - Works fine from what I tested David. And the code looks great. It's almost exactly what I had written before you posted this. Only I had more variations. I don't think the extras I came up with are really required though. Will push yours for integration now.
          Hide
          Jason Fowler added a comment -

          Shown this to Martin, will be adding a third, higher res size for the forms.

          Show
          Jason Fowler added a comment - Shown this to Martin, will be adding a third, higher res size for the forms.
          Hide
          Jason Fowler added a comment -

          Okay, added a third, wider label width, and requesting a peer review now.

          Show
          Jason Fowler added a comment - Okay, added a third, wider label width, and requesting a peer review now.
          Hide
          Damyon Wiese added a comment -

          This looks good to me Jason and David.

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

          Two things I did notice when checking against the documentation for less, is that in the latest beta they have changed some syntax so that:

          () are required around all operations (so the additions in this patch will need changing when we upgrade less) and all arguments to operations need to be the same (not related to this patch but I spotted some existing ones like "@horizontalComponentOffset - 20;").

          +1 for integration.

          Show
          Damyon Wiese added a comment - This looks good to me Jason and David. [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Two things I did notice when checking against the documentation for less, is that in the latest beta they have changed some syntax so that: () are required around all operations (so the additions in this patch will need changing when we upgrade less) and all arguments to operations need to be the same (not related to this patch but I spotted some existing ones like "@horizontalComponentOffset - 20;"). +1 for integration.
          Hide
          Damyon Wiese added a comment - - edited

          MDL-39341 needs integrating so I'm going to do it first and then provide a rebased patch for this issue.

          Show
          Damyon Wiese added a comment - - edited MDL-39341 needs integrating so I'm going to do it first and then provide a rebased patch for this issue.
          Hide
          Damyon Wiese added a comment -

          Pushed the rebased branch (no changes - just based on top of MDL-39341)

          Show
          Damyon Wiese added a comment - Pushed the rebased branch (no changes - just based on top of MDL-39341 )
          Hide
          Dan Poltawski added a comment -

          Integrated to master - thanks.

          Show
          Dan Poltawski added a comment - Integrated to master - thanks.
          Hide
          Mark Nelson added a comment -

          That's one nice looking theme! Well done, passing.

          Show
          Mark Nelson added a comment - That's one nice looking theme! Well done, passing.
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao
          Hide
          David Scotson added a comment -

          While looking at MDL-39424 I noticed that this sets 3 different sizes, but only two ever get used.

          < 768px the forms stack vertically so label width isn't used.
          768-980px has 200px set as the width
          > 980px has 245px set as the width

          The default width is also set to 180px but because the above rules cover the entire range it's always overruled and never actually used. I think the intention was to start from 768px with 180px labels and then have two further steps up as the screen gets wider. Also, we go to maximum width before we even get to the size of my tiny netbook screen, so probably the two higher steps need to trigger much wider.

          As discovered in MDL-39424 testing with blocks on both sides and wide form content (HTML editor, file upload) is a good stress test of how wide you can go.

          Show
          David Scotson added a comment - While looking at MDL-39424 I noticed that this sets 3 different sizes, but only two ever get used. < 768px the forms stack vertically so label width isn't used. 768-980px has 200px set as the width > 980px has 245px set as the width The default width is also set to 180px but because the above rules cover the entire range it's always overruled and never actually used. I think the intention was to start from 768px with 180px labels and then have two further steps up as the screen gets wider. Also, we go to maximum width before we even get to the size of my tiny netbook screen, so probably the two higher steps need to trigger much wider. As discovered in MDL-39424 testing with blocks on both sides and wide form content (HTML editor, file upload) is a good stress test of how wide you can go.
          Hide
          David Scotson added a comment -

          Should I re-open this bug for the issue above, or create a new one?

          Show
          David Scotson added a comment - Should I re-open this bug for the issue above, or create a new one?
          Hide
          Jason Fowler added a comment -

          Hi David, please create a new one, and put me as the assignee and I will follow up on it.

          Show
          Jason Fowler added a comment - Hi David, please create a new one, and put me as the assignee and I will follow up on it.
          Hide
          Jason Fowler added a comment -

          Second thoughts, I'll raise the issue and follow up on it now.

          Show
          Jason Fowler added a comment - Second thoughts, I'll raise the issue and follow up on it now.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: