Moodle
  1. Moodle
  2. MDL-41175

Add/remove group members page broken on small screens

    Details

    • Testing Instructions:
      Hide

      NOTE TO TESTER:
      This test requires groups. Should particularly be tested in firefox.

      1. Test in both clean and standard themes
      2. TEST that when adding/removing group members that the add/remove buttons are visible whatever the screen resolution
      Show
      NOTE TO TESTER: This test requires groups. Should particularly be tested in firefox. Test in both clean and standard themes TEST that when adding/removing group members that the add/remove buttons are visible whatever the screen resolution
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-41175_M26

      Description

      A screenshot will describe this best.

      As screen sizes narrow the buttons on the add/remove group members page start to disappear, at least on firefox.

        Gliffy Diagrams

        1. addremove.png
          37 kB
        2. danp-nofix.png
          217 kB
        3. MDL-41175.jpg
          28 kB
        4. MDL-41175-with.png
          47 kB
        5. MDL-41175-without.png
          48 kB
        6. MDL-41175-without-well.png
          73 kB
        7. MDL-41175-with-well.png
          66 kB

          Issue Links

            Activity

            Hide
            Mary Evans added a comment -

            Thanks for this Damyon, just when I thought things were getting better!

            Show
            Mary Evans added a comment - Thanks for this Damyon, just when I thought things were getting better!
            Hide
            Mary Evans added a comment -

            Just spent an hour or two setting a group up and adding some more students to a course and adding them to a group.
            While I was doing this I tested Standard theme and found that it looks much the same as Clean theme does.

            From memory it's not a new problem and not necessarily something that needs fixing in themes. It has more to do with the way the lists have been created. It would look better if the lists were positioned one above the other. This way both lists would show the complete email addresses and it would make it much easier to style.

            Show
            Mary Evans added a comment - Just spent an hour or two setting a group up and adding some more students to a course and adding them to a group. While I was doing this I tested Standard theme and found that it looks much the same as Clean theme does. From memory it's not a new problem and not necessarily something that needs fixing in themes. It has more to do with the way the lists have been created. It would look better if the lists were positioned one above the other. This way both lists would show the complete email addresses and it would make it much easier to style.
            Hide
            Mary Evans added a comment -

            Closing this as it is impossible to fix anything in clean theme as it looks OK in a wide screen.

            Show
            Mary Evans added a comment - Closing this as it is impossible to fix anything in clean theme as it looks OK in a wide screen.
            Hide
            Mary Evans added a comment -

            Just adding a link to MDL-41195 which is an improvement to change the way groups are displayed.

            Show
            Mary Evans added a comment - Just adding a link to MDL-41195 which is an improvement to change the way groups are displayed.
            Hide
            Damyon Wiese added a comment -

            I'm not sure this should have been closed - it only looks broken like this in firefox (chrome shows the full text of the buttons).

            Show
            Damyon Wiese added a comment - I'm not sure this should have been closed - it only looks broken like this in firefox (chrome shows the full text of the buttons).
            Hide
            Mary Evans added a comment -

            Damyon, If you have time can you Peer Review this please?

            Show
            Mary Evans added a comment - Damyon, If you have time can you Peer Review this please?
            Hide
            Damyon Wiese added a comment -

            Hi Mary - thanks for looking at this.

            I tested that fix - but it did not resolve it for me.

            I did some quick testing at the same time and this seems to work for me:

            +++ b/theme/base/style/core.css
            @@ -347,7 +347,7 @@ input#id_externalurl {direction:ltr;}
             .groupmanagementtable #existingcell,
             .groupmanagementtable #potentialcell {width: 42%;}
             .groupmanagementtable #buttonscell {width: 16%;}
            -.groupmanagementtable #buttonscell input {width: 80%;}
            +.groupmanagementtable #buttonscell input {min-width: 80%;}
             .groupmanagementtable #removeselect_wrapper,
             .groupmanagementtable #addselect_wrapper {width: 100%;}
             .groupmanagementtable #removeselect_wrapper label,
            
            

            With this change - at full size the add/remove buttons are the same size, and on smaller widths - the add button is smaller than the remove button - but the text is at least always visible.

            Cheers - Damyon

            Show
            Damyon Wiese added a comment - Hi Mary - thanks for looking at this. I tested that fix - but it did not resolve it for me. I did some quick testing at the same time and this seems to work for me: +++ b/theme/base/style/core.css @@ -347,7 +347,7 @@ input#id_externalurl {direction:ltr;} .groupmanagementtable #existingcell, .groupmanagementtable #potentialcell {width: 42%;} .groupmanagementtable #buttonscell {width: 16%;} -.groupmanagementtable #buttonscell input {width: 80%;} +.groupmanagementtable #buttonscell input {min-width: 80%;} .groupmanagementtable #removeselect_wrapper, .groupmanagementtable #addselect_wrapper {width: 100%;} .groupmanagementtable #removeselect_wrapper label, With this change - at full size the add/remove buttons are the same size, and on smaller widths - the add button is smaller than the remove button - but the text is at least always visible. Cheers - Damyon
            Hide
            Mary Evans added a comment -

            OK...thanks Damyon.

            As per your instructions I have made the changes in base/style/core.css & bootstrapbase/less/moodle/core.less

            Pushing this for integration review.

            Cheers
            Mary

            Show
            Mary Evans added a comment - OK...thanks Damyon. As per your instructions I have made the changes in base/style/core.css & bootstrapbase/less/moodle/core.less Pushing this for integration review. Cheers Mary
            Hide
            Gareth J Barnard added a comment -

            Will this be back ported to M2.5?

            Show
            Gareth J Barnard added a comment - Will this be back ported to M2.5?
            Hide
            Mary Evans added a comment -

            I doubt it as I don't see how this can fix the problem, but since Damyon said my original fix did not work, but by just adding a min-width to the input button he thought worked best, see his comment above. So that is what I have done. Personally I think he got it wrong. But who am I to argue!

            Show
            Mary Evans added a comment - I doubt it as I don't see how this can fix the problem, but since Damyon said my original fix did not work, but by just adding a min-width to the input button he thought worked best, see his comment above. So that is what I have done. Personally I think he got it wrong. But who am I to argue!
            Hide
            Gareth J Barnard added a comment -

            Hi Mary,

            Was your original fix:

            .groupmanagementtable #buttonscell input {width: 80%;}
            

            ? As I cannot see the original fix. If so, then I would have thought it would work

            Show
            Gareth J Barnard added a comment - Hi Mary, Was your original fix: .groupmanagementtable #buttonscell input {width: 80%;} ? As I cannot see the original fix. If so, then I would have thought it would work
            Hide
            Dan Poltawski added a comment -

            Hi Everyone,

            Still doesn't look like this is fixed to me. I can reproduce the same problem on firefox.

            Attached screenshot with the inspector open, showing that min-width fix is there, but its not fixing the issue.

            Show
            Dan Poltawski added a comment - Hi Everyone, Still doesn't look like this is fixed to me. I can reproduce the same problem on firefox. Attached screenshot with the inspector open, showing that min-width fix is there, but its not fixing the issue.
            Hide
            Dan Poltawski added a comment -

            (Also look at that horrible border around the description)

            Show
            Dan Poltawski added a comment - (Also look at that horrible border around the description)
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Mary Evans added a comment -

            LOL...I knew this would happen. Groups needs cleaning up altogether, but I'll see what I can do.

            Show
            Mary Evans added a comment - LOL...I knew this would happen. Groups needs cleaning up altogether, but I'll see what I can do.
            Hide
            Mary Evans added a comment - - edited

            @Dan,

            If this patch is acceptable I'll cherry-pick to MDL-41175_M25

            Thanks

            Show
            Mary Evans added a comment - - edited @Dan, If this patch is acceptable I'll cherry-pick to MDL-41175 _M25 Thanks
            Hide
            Mary Evans added a comment -

            @Gareth,
            No my changes where mainly in group/members.php which is really badly in need of a complete face lift!

            Show
            Mary Evans added a comment - @Gareth, No my changes where mainly in group/members.php which is really badly in need of a complete face lift!
            Hide
            Gareth J Barnard added a comment -

            Mary Evans,

            Thanks, shame they were not accepted as would have been a golden opportunity for progression where a bug is solved by making things better.

            A question, why do you have the 'well' class when it's not used?...

            $groupinfotable->attributes['class'] = 'groupinfobox well';
            

            And are 'wells' a CSS thing or a Bootstrap thing?

            Ta,

            Gareth

            Show
            Gareth J Barnard added a comment - Mary Evans , Thanks, shame they were not accepted as would have been a golden opportunity for progression where a bug is solved by making things better. A question, why do you have the 'well' class when it's not used?... $groupinfotable->attributes['class'] = 'groupinfobox well'; And are 'wells' a CSS thing or a Bootstrap thing? Ta, Gareth
            Hide
            Mary Evans added a comment - - edited

            it's a Bootstrap class which styles a box giving it rounder corners a light grey background and a light coloured border.

            It's not used in standard Moodle themes so should not affect it, but works great in Clean theme.
            See image when I post it.

            If it is 'well liked' (pardon the pun but am I bovvered!) we can add CSS to Base theme

            Show
            Mary Evans added a comment - - edited it's a Bootstrap class which styles a box giving it rounder corners a light grey background and a light coloured border. It's not used in standard Moodle themes so should not affect it, but works great in Clean theme. See image when I post it. If it is 'well liked' (pardon the pun but am I bovvered!) we can add CSS to Base theme
            Hide
            Gareth J Barnard added a comment -

            Ha ha!

            If it's a Bootstrap specific thing, then in LESS you can include a class within a class, so for core.less:

            .groupmanagementtable #buttonscell input {
                width: 80%;
                min-width: 80px;
                .well;
            }
            

            Then Moodle can stay pure and Bootstrap can add it's own styling.

            Show
            Gareth J Barnard added a comment - Ha ha! If it's a Bootstrap specific thing, then in LESS you can include a class within a class, so for core.less: .groupmanagementtable #buttonscell input { width: 80%; min-width: 80px; .well; } Then Moodle can stay pure and Bootstrap can add it's own styling.
            Hide
            Mary Evans added a comment -

            If you take look at MDL-41195 you will see Damyon's remarks which is, on the face of it, good news. So this little fix here is only a temporary thing. At least that's how I am looking at it.

            I've just added the image https://tracker.moodle.org/secure/attachment/34161/MDL-41175.jpg

            Show
            Mary Evans added a comment - If you take look at MDL-41195 you will see Damyon's remarks which is, on the face of it, good news. So this little fix here is only a temporary thing. At least that's how I am looking at it. I've just added the image https://tracker.moodle.org/secure/attachment/34161/MDL-41175.jpg
            Hide
            Damyon Wiese added a comment -

            Hi Mary -

            This does not match what I suggested. - the format of my comment may have thrown you off (it was a diff from git).

            "min-width: 80px" should be 80% (small visual difference)

            "width: 80%" - this line must go - it is causing the breakage because it is making the input smaller than the text inside it - we should not set the width - only the min-width.

            -1 for the extra change (.well)

            Show
            Damyon Wiese added a comment - Hi Mary - This does not match what I suggested. - the format of my comment may have thrown you off (it was a diff from git). "min-width: 80px" should be 80% (small visual difference) "width: 80%" - this line must go - it is causing the breakage because it is making the input smaller than the text inside it - we should not set the width - only the min-width. -1 for the extra change (.well)
            Hide
            Mary Evans added a comment - - edited

            I did not set width of the input, it already exists in Moodle/theme/base/style/core.css so please don't tell me not to set widths.

            .groupmanagementtable #buttonscell {width: 16%;} 
            .groupmanagementtable #buttonscell input {width: 80%;}
            

            So in effect the input Add/Remove buttons represents 80% of 16%
            so mathematically speaking if 16% = 100px the input button would be 80px which is about right. This in effect forces the buttoncell to stay open instead of closing up.

            So I am not quite sure why you are saying it is breaking something.

            Show
            Mary Evans added a comment - - edited I did not set width of the input, it already exists in Moodle/theme/base/style/core.css so please don't tell me not to set widths. .groupmanagementtable #buttonscell {width: 16%;} .groupmanagementtable #buttonscell input {width: 80%;} So in effect the input Add/Remove buttons represents 80% of 16% so mathematically speaking if 16% = 100px the input button would be 80px which is about right. This in effect forces the buttoncell to stay open instead of closing up. So I am not quite sure why you are saying it is breaking something.
            Hide
            Mary Evans added a comment -

            Oh and the "well" to style the Description box, which I added on Bootstrap's behalf as this is in keeping with what Martin said about adding new classes for Bootstrap.

            Show
            Mary Evans added a comment - Oh and the "well" to style the Description box, which I added on Bootstrap's behalf as this is in keeping with what Martin said about adding new classes for Bootstrap.
            Hide
            Mary Evans added a comment -

            I am actually wondering if you are testing this or just looking at the CSS and not really seeing the whole picture.

            Show
            Mary Evans added a comment - I am actually wondering if you are testing this or just looking at the CSS and not really seeing the whole picture.
            Hide
            Gareth J Barnard added a comment - - edited

            Hi,

            I applied the '.less' changes part of the patch pertaining to the width on Moodle 2.5 and discovered on Firefox 23.0.1 that the 'width: 80%' IS required to make the buttons line up - please see MDL-41175-with.png and MDL-41175-without.png.

            Also, I applied the thinking with the 'well' class through changing 'bootstrapbase/less/moodle/users.less' to:

            .groupinfobox {
                border: 1px solid;
                .well;
            }
            

            As you can see from MDL-41175-well.png and MDL-41175-without-well.png, the change IS required to make the page appearance the same in line with the rest of the style of the theme.

            Cheers,

            Gareth

            P.S. 'width: 80%' only makes the button smaller if you don't have 'min-width: 80%' - when you have 'both' then you have a solution.

            Show
            Gareth J Barnard added a comment - - edited Hi, I applied the '.less' changes part of the patch pertaining to the width on Moodle 2.5 and discovered on Firefox 23.0.1 that the 'width: 80%' IS required to make the buttons line up - please see MDL-41175 -with.png and MDL-41175 -without.png. Also, I applied the thinking with the 'well' class through changing 'bootstrapbase/less/moodle/users.less' to: .groupinfobox { border: 1px solid; .well; } As you can see from MDL-41175 -well.png and MDL-41175 -without-well.png, the change IS required to make the page appearance the same in line with the rest of the style of the theme. Cheers, Gareth P.S. 'width: 80%' only makes the button smaller if you don't have 'min-width: 80%' - when you have 'both' then you have a solution.
            Hide
            Mary Evans added a comment - - edited

            That's why I added the class to members.php as it requires no css changes!

            And that is the beauty of Bootstrap!

            Show
            Mary Evans added a comment - - edited That's why I added the class to members.php as it requires no css changes! And that is the beauty of Bootstrap!
            Hide
            Gareth J Barnard added a comment -

            But - making the CSS change in Bootstrap 'only' means that other theme's don't have a redundant 'well' class in the markup. To be consistent, the 'base' theme would need the 'well' class CSS adding to it.

            Show
            Gareth J Barnard added a comment - But - making the CSS change in Bootstrap 'only' means that other theme's don't have a redundant 'well' class in the markup. To be consistent, the 'base' theme would need the 'well' class CSS adding to it.
            Hide
            Mary Evans added a comment - - edited

            Gareth, we need to start as we mean to go on if Bootstrap is to work as it should. At present we are slowly slipping back into fixing themes when the real problem is in CORE.

            As for adding one tiny css class to the html within a core file is hardly a problem and it is not going to affect anything in standard themes by its presence there.

            Read Item 5. in this CSS Policy document that Martin authored.
            https://tracker.moodle.org/browse/MDL-40187?page=com.atlassian.jira.plugin.system.issuetabpanels:changehistory-tabpanel

            Show
            Mary Evans added a comment - - edited Gareth, we need to start as we mean to go on if Bootstrap is to work as it should. At present we are slowly slipping back into fixing themes when the real problem is in CORE. As for adding one tiny css class to the html within a core file is hardly a problem and it is not going to affect anything in standard themes by its presence there. Read Item 5. in this CSS Policy document that Martin authored. https://tracker.moodle.org/browse/MDL-40187?page=com.atlassian.jira.plugin.system.issuetabpanels:changehistory-tabpanel
            Hide
            Mary Evans added a comment -

            Just added David Scotson for a bit of back up on this argument.

            Show
            Mary Evans added a comment - Just added David Scotson for a bit of back up on this argument.
            Hide
            Gareth J Barnard added a comment -

            Ok, to clarify things the way I see them from a software engineering point of view:

            1. Moving to renderers is a good thing.
            2. The 'well' class is a Bootstrap entity in this context and as such should be scoped and encapsulated as such.
            3. The mark-up should define it's own set of identifiers / classes pertaining to the functionality of the interface that needs to be styled. This should be kept to a minimum with as much reuse as possible.
            4. Each framework defines its own mapping to implement '3' - Bootstrap has this with the 'moodle/less' folder. In this way you have a good one (core) to many (frameworks) mapping where frameworks can come and go but the core can have a longer more persistent longevity.
            5. "As for adding one tiny css class to the html within a core file is hardly a problem and it is not going to affect anything in standard themes by its presence there" does set a precedence and is in effect the key to Pandora's box.
            6. Item '5' although policy could be wrong.
            7. We all can have an opinion where each one is a valid solution. Often the best is a hybrid.

            This has now gone so far off course when the original intent was to justify the need to have 'width: 80%' as per the screen shots I uploaded.

            Gareth

            Show
            Gareth J Barnard added a comment - Ok, to clarify things the way I see them from a software engineering point of view: 1. Moving to renderers is a good thing. 2. The 'well' class is a Bootstrap entity in this context and as such should be scoped and encapsulated as such. 3. The mark-up should define it's own set of identifiers / classes pertaining to the functionality of the interface that needs to be styled. This should be kept to a minimum with as much reuse as possible. 4. Each framework defines its own mapping to implement '3' - Bootstrap has this with the 'moodle/less' folder. In this way you have a good one (core) to many (frameworks) mapping where frameworks can come and go but the core can have a longer more persistent longevity. 5. "As for adding one tiny css class to the html within a core file is hardly a problem and it is not going to affect anything in standard themes by its presence there" does set a precedence and is in effect the key to Pandora's box. 6. Item '5' although policy could be wrong. 7. We all can have an opinion where each one is a valid solution. Often the best is a hybrid. This has now gone so far off course when the original intent was to justify the need to have 'width: 80%' as per the screen shots I uploaded. Gareth
            Hide
            David Scotson added a comment -

            The table has four columns and the sizes of three of them are set to add up to 100% (42 + 16 + 42). The last (unsized) column mostly contains a div set to 14em width. This is because it's a slightly bodged version of the usual 'add to' screen, which doesn't have a 4th column.

            If I fix those issues, i.e. remove the 14em width rule, and set the two columns that are currently 42% wide to 30% wide then it all seems to work for me on any relevant screen size (ipad vertical/horizontal, netbook) though the buttons still almost disappear in Firefox between about 760-860px width and again at below 660px so it's still unusable on phone, whether vertical or horizontal.

            I'm not sure any amount of fiddling about with button sizes is going to make this screen usable on a phone though. It should be relatively easy to stop using tables for layout and put this in responsive grid columns though.

            Note the group info above this with the ugly black border (I thought I'd removed all the nasty 1px black borders when porting from Base, must have missed one) is also a table used for layout purposes, which would also benefit from using a responsive grid. Unfortunately it's not in a renderer which makes it a bit political to change.

            Show
            David Scotson added a comment - The table has four columns and the sizes of three of them are set to add up to 100% (42 + 16 + 42). The last (unsized) column mostly contains a div set to 14em width. This is because it's a slightly bodged version of the usual 'add to' screen, which doesn't have a 4th column. If I fix those issues, i.e. remove the 14em width rule, and set the two columns that are currently 42% wide to 30% wide then it all seems to work for me on any relevant screen size (ipad vertical/horizontal, netbook) though the buttons still almost disappear in Firefox between about 760-860px width and again at below 660px so it's still unusable on phone, whether vertical or horizontal. I'm not sure any amount of fiddling about with button sizes is going to make this screen usable on a phone though. It should be relatively easy to stop using tables for layout and put this in responsive grid columns though. Note the group info above this with the ugly black border (I thought I'd removed all the nasty 1px black borders when porting from Base, must have missed one) is also a table used for layout purposes, which would also benefit from using a responsive grid. Unfortunately it's not in a renderer which makes it a bit political to change.
            Hide
            Mary Evans added a comment -

            Thanks David, but what about the 'well' debate? Do you think this should be in CORE or just like we would do in Base theme fix it in the theme CSS? Just referring back to your comment in a previous discussion about ' some Crazy Themer' fixing CSS when the problem is really in core.

            Show
            Mary Evans added a comment - Thanks David, but what about the 'well' debate? Do you think this should be in CORE or just like we would do in Base theme fix it in the theme CSS? Just referring back to your comment in a previous discussion about ' some Crazy Themer' fixing CSS when the problem is really in core.
            Hide
            David Scotson added a comment -

            If I was changing the core code then I'd probably put the add to group functionality into grid columns, and I'd replace the image and text for the group with a Bootstrap Media Object:

            http://getbootstrap.com/2.3.2/components.html#media

            And I'd put it inside an offset column so that it would be centered and expand/contract with screen size on larger screens, and flip to 100% width on small screens.

            http://getbootstrap.com/2.3.2/scaffolding.html#fluidGridSystem

            And then backport whatever CSS was required to make this work in Base and child themes.

            Wells should be used to highlight stuff, but if you highlight everything then you just create noise. It's easy to look at standard Moodle and how everything has a box around it, and boxes translate easily to wells, but if you lay things out nicely then many of those boxes are unnecessary.

            Having said that, I agree with your assessment of Martin's decision, that when appropriate Bootstrap classes can just be added to core code, and that would apply to my suggestion of adding responsive grids and media objects above. Even still, I think it's a political minefield if you don't have a renderer to make the changes in though.

            Show
            David Scotson added a comment - If I was changing the core code then I'd probably put the add to group functionality into grid columns, and I'd replace the image and text for the group with a Bootstrap Media Object: http://getbootstrap.com/2.3.2/components.html#media And I'd put it inside an offset column so that it would be centered and expand/contract with screen size on larger screens, and flip to 100% width on small screens. http://getbootstrap.com/2.3.2/scaffolding.html#fluidGridSystem And then backport whatever CSS was required to make this work in Base and child themes. Wells should be used to highlight stuff, but if you highlight everything then you just create noise. It's easy to look at standard Moodle and how everything has a box around it, and boxes translate easily to wells, but if you lay things out nicely then many of those boxes are unnecessary. Having said that, I agree with your assessment of Martin's decision, that when appropriate Bootstrap classes can just be added to core code, and that would apply to my suggestion of adding responsive grids and media objects above. Even still, I think it's a political minefield if you don't have a renderer to make the changes in though.
            Hide
            Mary Evans added a comment -

            @David,

            Well, we better start learning how to create a renderer from scratch, then we can fix stuff, and help make Moodle 3.0 something spectacular.

            Show
            Mary Evans added a comment - @David, Well, we better start learning how to create a renderer from scratch, then we can fix stuff, and help make Moodle 3.0 something spectacular.
            Hide
            Mary Evans added a comment -

            @Damyon,

            I am concerned that the reason you are not seeing what we see, is that you may be testing this in Moodle 2.5 whereas this is based on master.

            I am only telling you this as it has happened before.

            Cheers
            Mary

            Show
            Mary Evans added a comment - @Damyon, I am concerned that the reason you are not seeing what we see, is that you may be testing this in Moodle 2.5 whereas this is based on master . I am only telling you this as it has happened before. Cheers Mary
            Hide
            Gareth J Barnard added a comment -

            My screen shots are based on Moodle 2.5 and demonstrate that 'width: 80%' is required.

            Show
            Gareth J Barnard added a comment - My screen shots are based on Moodle 2.5 and demonstrate that 'width: 80%' is required.
            Hide
            David Scotson added a comment -

            The two buttons don't line up because as input type=submit's they're picking up 5px of left margin (required to make forms in totally seperate parts of Moodle work). The bottom one then has margin: 3em auto 5em applied to space the buttons out a bit better. This overrides the 5px left margin for the buttom one, but not the top one. You can fix that directly, without setting the width to 80%, which I assume only works by accident (it doesn't look lined up on my machine with width: 80% applied, on Ubuntu Firefox) by setting both to have auto margins left and right.

            Just noticed that as well as using tables for layout, this particular table describes itself with the class "generaltable", so it picks up various styles intended for, um, general tables. Amongst other things this is adding a bit of padding, when we're already tight for space.

            Show
            David Scotson added a comment - The two buttons don't line up because as input type=submit's they're picking up 5px of left margin (required to make forms in totally seperate parts of Moodle work). The bottom one then has margin: 3em auto 5em applied to space the buttons out a bit better. This overrides the 5px left margin for the buttom one, but not the top one. You can fix that directly, without setting the width to 80%, which I assume only works by accident (it doesn't look lined up on my machine with width: 80% applied, on Ubuntu Firefox) by setting both to have auto margins left and right. Just noticed that as well as using tables for layout, this particular table describes itself with the class "generaltable", so it picks up various styles intended for, um, general tables. Amongst other things this is adding a bit of padding, when we're already tight for space.
            Hide
            Gareth J Barnard added a comment -

            Pants. I think David Scotson that the button alignment from your description is similar to MDL-39820.

            Show
            Gareth J Barnard added a comment - Pants. I think David Scotson that the button alignment from your description is similar to MDL-39820 .
            Hide
            Mary Evans added a comment -

            @Gareth, did you do git merge FETCH_HEAD from the branch listed above?

            I would have expected it to have caused lots of conflicts.

            Show
            Mary Evans added a comment - @Gareth, did you do git merge FETCH_HEAD from the branch listed above? I would have expected it to have caused lots of conflicts.
            Hide
            Gareth J Barnard added a comment -

            Mary Evans, umm, no, the changes were so small (members.php and core.less with a quick 'recess') that I made them myself using my IDE on my own MOODLE_25_STABLE branch that was up to date.

            Show
            Gareth J Barnard added a comment - Mary Evans , umm, no, the changes were so small (members.php and core.less with a quick 'recess') that I made them myself using my IDE on my own MOODLE_25_STABLE branch that was up to date.
            Hide
            Mary Evans added a comment -

            I'm going to fix this as required by Damyon.

            Show
            Mary Evans added a comment - I'm going to fix this as required by Damyon.
            Hide
            Mary Evans added a comment - - edited

            I'll create a new issue for adding the well to the description box, so as not to confuse matters.

            Thanks Gareth for reminding me how to add the .well as a css rule inside existing LESS mark-up.

            Show
            Mary Evans added a comment - - edited I'll create a new issue for adding the well to the description box, so as not to confuse matters. Thanks Gareth for reminding me how to add the .well as a css rule inside existing LESS mark-up.
            Hide
            Mary Evans added a comment - - edited

            I found the fix Damyon was talking about...he just gave me the wrong CSS!

            p.arrow_button input {
                -moz-box-sizing: border-box;
                display: block;
                min-width: 80%;
                padding-left: 0;
                padding-right: 0;
            }

            added to .groupmanagementtable #buttonscell works as required.

            .groupmanagementtable #buttonscell p.arrow_button input {
                min-width: 80%;
            }
            

            Show
            Mary Evans added a comment - - edited I found the fix Damyon was talking about...he just gave me the wrong CSS! p.arrow_button input { -moz-box-sizing: border-box; display: block; min-width: 80%; padding-left: 0; padding-right: 0; } added to .groupmanagementtable #buttonscell works as required. .groupmanagementtable #buttonscell p.arrow_button input { min-width: 80%; }
            Hide
            Mary Evans added a comment -

            Damyon, hope you can find time to Peer Review this.

            Thanks

            Mary

            Show
            Mary Evans added a comment - Damyon, hope you can find time to Peer Review this. Thanks Mary
            Hide
            David Scotson added a comment -

            I add six bugs that this revealed. If the fix for the current issue (or one of the linked bugs) doesn't resolve the fact that the table used for layout has sizes that add up to greater 100% then I'll add a bug for that too.

            Show
            David Scotson added a comment - I add six bugs that this revealed. If the fix for the current issue (or one of the linked bugs) doesn't resolve the fact that the table used for layout has sizes that add up to greater 100% then I'll add a bug for that too.
            Hide
            David Scotson added a comment -

            I made a few changes to better reflect the issue now it's been investigated.

            Does this really deserve the critical flag? There's a lot of corners in Moodle that doesn't work on small screens or touch devices.

            Are the proposed fixes intended to work on phones? The testing instructions say "whatever the screen resolution" but I think a short term fix that worked only on tablets, or at least landscape phones might be acceptable.

            Show
            David Scotson added a comment - I made a few changes to better reflect the issue now it's been investigated. Does this really deserve the critical flag? There's a lot of corners in Moodle that doesn't work on small screens or touch devices. Are the proposed fixes intended to work on phones? The testing instructions say "whatever the screen resolution" but I think a short term fix that worked only on tablets, or at least landscape phones might be acceptable.
            Hide
            Gareth J Barnard added a comment -

            So give the fact that it has a table and this is bad, should not Mary Evans's original solution to revamp the layout without tables stand? Surely this is a better long term fix. On the theme's forum I spotted that Mary had mentioned about CSS Zen Garden, then in reading 'Avoid the top 10 CSS mistakes' in September 2013's .Net magazine (http://www.netmagazine.com/shop/magazines/september-2013-245) where it states that "we've come along way from using the CSS Zen Garden to convince the web to go table-less". So therefore this code must be really out of date. Therefore fix it now to solve this problem and reduce the paperwork in yet more tracker issues.

            Show
            Gareth J Barnard added a comment - So give the fact that it has a table and this is bad, should not Mary Evans 's original solution to revamp the layout without tables stand? Surely this is a better long term fix. On the theme's forum I spotted that Mary had mentioned about CSS Zen Garden, then in reading 'Avoid the top 10 CSS mistakes' in September 2013's .Net magazine ( http://www.netmagazine.com/shop/magazines/september-2013-245 ) where it states that "we've come along way from using the CSS Zen Garden to convince the web to go table-less". So therefore this code must be really out of date. Therefore fix it now to solve this problem and reduce the paperwork in yet more tracker issues.
            Hide
            David Scotson added a comment -

            Well, yes and no, replacing this table layout with something that works well on a variety of devices including mobile touch is clearly the right thing to do in the long term.

            On the other hand, it's still not clear to me that this current bug is even an actual problem for anyone who isn't randomly re-sizing their firefox window to weird sizes (in which case the workaround is to make the screen bigger), so Mary was probably right in closing it earlier. It works fine for me in Chrome at iPad sizes, and is possibly even usable with a landscape iPhone, but someone probably needs to test with actual devices to see exactly how broken this screen is on such devices and what can be done about it if that is the case. (I personally use Firefox on an Android tablet which might suffer particularly badly, but that's fairly niche).

            Did Mary actually propose something that involved removing either of the the tables on this page? I didn't see that, and I'm not sure it's an immediate priority since core Moodle has no real solution for using grid layouts yet, and this area isn't in a renderer that would allow Bootstrapbase(d) themes to do it themselves, so you'd be doing major surgery on a bit of Moodle that's possible not even causing anyone any problems with no clear steer on how Moodle core wants to approach these issues.

            I do on the other hand see several very small, targeted fixes that could easily be made here though that would have immediate impact i.e. getting rid of the ugly black border on the description, removing the .generaltable class from this and similar screens, making the add/remove buttons line up in this and similar screens.

            Show
            David Scotson added a comment - Well, yes and no, replacing this table layout with something that works well on a variety of devices including mobile touch is clearly the right thing to do in the long term. On the other hand, it's still not clear to me that this current bug is even an actual problem for anyone who isn't randomly re-sizing their firefox window to weird sizes (in which case the workaround is to make the screen bigger), so Mary was probably right in closing it earlier. It works fine for me in Chrome at iPad sizes, and is possibly even usable with a landscape iPhone, but someone probably needs to test with actual devices to see exactly how broken this screen is on such devices and what can be done about it if that is the case. (I personally use Firefox on an Android tablet which might suffer particularly badly, but that's fairly niche). Did Mary actually propose something that involved removing either of the the tables on this page? I didn't see that, and I'm not sure it's an immediate priority since core Moodle has no real solution for using grid layouts yet, and this area isn't in a renderer that would allow Bootstrapbase(d) themes to do it themselves, so you'd be doing major surgery on a bit of Moodle that's possible not even causing anyone any problems with no clear steer on how Moodle core wants to approach these issues. I do on the other hand see several very small, targeted fixes that could easily be made here though that would have immediate impact i.e. getting rid of the ugly black border on the description, removing the .generaltable class from this and similar screens, making the add/remove buttons line up in this and similar screens.
            Hide
            Gareth J Barnard added a comment -

            I read somewhere that Mary Evans stated that the groups code needed a serious re-write.

            Show
            Gareth J Barnard added a comment - I read somewhere that Mary Evans stated that the groups code needed a serious re-write.
            Hide
            David Scotson added a comment -

            Well I can definitely agree with that, but if someone was looking for a quick win where they could make an actual difference to user experience of Moodle, I'd point them to some smaller tasks (as listed above) or even to some bigger tasks where the way forward is a bit more mapped out e.g. the bits of Moodle that are already in renderers and writing ones that emit Bootstrap-compatible HTML.

            Show
            David Scotson added a comment - Well I can definitely agree with that, but if someone was looking for a quick win where they could make an actual difference to user experience of Moodle, I'd point them to some smaller tasks (as listed above) or even to some bigger tasks where the way forward is a bit more mapped out e.g. the bits of Moodle that are already in renderers and writing ones that emit Bootstrap-compatible HTML.
            Hide
            Gareth J Barnard added a comment -

            Are some of the tasks listed above already open tracker issues?

            Show
            Gareth J Barnard added a comment - Are some of the tasks listed above already open tracker issues?
            Hide
            Mary Evans added a comment - - edited

            I read somewhere that Gareth J Barnard said that Mary Evans stated that the groups code needed a serious re-write. It was probably in an email when I was having a moan! LOL

            See MDL-41195 for more info regarding making Group pages more responsive.

            Show
            Mary Evans added a comment - - edited I read somewhere that Gareth J Barnard said that Mary Evans stated that the groups code needed a serious re-write. It was probably in an email when I was having a moan! LOL See MDL-41195 for more info regarding making Group pages more responsive.
            Hide
            Mary Evans added a comment -
            Show
            Mary Evans added a comment - David Scotson has created more bugs
            Hide
            Mary Evans added a comment - - edited

            David have you seen my resent changes that I did last night?
            https://github.com/lazydaisy/moodle/compare/master...MDL-41175_master

            This fixes the button problem nicely.

            Show
            Mary Evans added a comment - - edited David have you seen my resent changes that I did last night? https://github.com/lazydaisy/moodle/compare/master...MDL-41175_master This fixes the button problem nicely.
            Hide
            David Scotson added a comment -

            I've not tried it yet, but in general if a fix involves adding more CSS to cover up problems caused by the HTML then it's not the best possible solution. One or two fixes like that is fine, but once they multiply you end up with complicated HTML and complicated CSS both of which are hard to work with and waste bandwidth/time to download.

            I've done plenty of workarounds like that myself in themes, and I'm sure more might be necessary sometimes, I just think it's a shame if a problem can't be fixed at the root cause.

            Show
            David Scotson added a comment - I've not tried it yet, but in general if a fix involves adding more CSS to cover up problems caused by the HTML then it's not the best possible solution. One or two fixes like that is fine, but once they multiply you end up with complicated HTML and complicated CSS both of which are hard to work with and waste bandwidth/time to download. I've done plenty of workarounds like that myself in themes, and I'm sure more might be necessary sometimes, I just think it's a shame if a problem can't be fixed at the root cause.
            Hide
            Mary Evans added a comment -

            I know, and all this discussion for such a petty thing is crazy.

            Show
            Mary Evans added a comment - I know, and all this discussion for such a petty thing is crazy.
            Hide
            Damyon Wiese added a comment -

            Thanks Mary,

            Testing this in firefox the patch does fix this display issue when using the clean theme.

            It does not however fix this in the standard theme, please also add the css changes to base so this fix applies to all themes.

            Also - this is a bug and needs to be backported to stables (only if it is this simple css fix - not if it is an elaborate rewrite of all the group screens).

            Cheers, Damyon

            Show
            Damyon Wiese added a comment - Thanks Mary, Testing this in firefox the patch does fix this display issue when using the clean theme. It does not however fix this in the standard theme, please also add the css changes to base so this fix applies to all themes. Also - this is a bug and needs to be backported to stables (only if it is this simple css fix - not if it is an elaborate rewrite of all the group screens). Cheers, Damyon
            Hide
            Mary Evans added a comment -

            Will do...thanks

            Show
            Mary Evans added a comment - Will do...thanks
            Hide
            Dan Poltawski added a comment -

            Sending this back for a peer review.

            Show
            Dan Poltawski added a comment - Sending this back for a peer review.
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Mary Evans added a comment -

            Damyon Wiese
            Asking for peer review if/when you can spare a moment or two?
            As you will see I have reduced the css slightly as was duplicating some Bootstrap(base) css

            Show
            Mary Evans added a comment - Damyon Wiese Asking for peer review if/when you can spare a moment or two? As you will see I have reduced the css slightly as was duplicating some Bootstrap(base) css
            Hide
            Mary Evans added a comment -

            I guess this is not going to make it into Moodle core this month?

            Show
            Mary Evans added a comment - I guess this is not going to make it into Moodle core this month?
            Hide
            Damyon Wiese added a comment -

            Thanks Mary - looks perfect to me sending for integration.

            I tested this in clean and standard on chrome, firefox, ie8 and ie10.

            The behaviour in all cases is that no matter how thin the window is resized, the full text of the buttons is readable.

            Show
            Damyon Wiese added a comment - Thanks Mary - looks perfect to me sending for integration. I tested this in clean and standard on chrome, firefox, ie8 and ie10. The behaviour in all cases is that no matter how thin the window is resized, the full text of the buttons is readable.
            Hide
            Dan Poltawski added a comment -

            thanks Mary, integrated to master, 25 and 24

            Show
            Dan Poltawski added a comment - thanks Mary, integrated to master, 25 and 24
            Hide
            Frédéric Massart added a comment -

            Test passed (Firefox, Chrome). Now the buttons are always clickable. However, please note that the table is still not responsive.

            Show
            Frédéric Massart added a comment - Test passed (Firefox, Chrome). Now the buttons are always clickable. However, please note that the table is still not responsive.
            Hide
            Mary Evans added a comment -

            Don't worry about the table Fred, we will get it down to a postage stamp size soon enough, probably with micro dots as icons! LOL

            Show
            Mary Evans added a comment - Don't worry about the table Fred, we will get it down to a postage stamp size soon enough, probably with micro dots as icons! LOL
            Hide
            Dan Poltawski added a comment -

            Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

            A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

            Show
            Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: