Details

      Description

      See attached screenshots.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            bawjaws David Scotson added a comment -

            There's a few odd things here:

            1) this should probably have the generaltable class, rather than generalbox. If it did, it would pick up the standard Bootstrap table styles with striping and generally look a little nicer.

            2) the width of the Rating column header is set to be 100% (of the width of the table) which is obviously not correct and seems to create chaos under certain circumstances. I think this is what's causing the name to overlap with the image sometimes.

            3) there's specific styles added to center align the data, but not the headers. But strangely, possibly due to number 2) above, all the columns but Rating look left-aligned.

            4) there's a no-wrap style set, which if you remove causes the table to become a mess, but again this seems to be a symptom of 2) above, once you remove that the table adapts to small screens without too much fuss

            So to me it looks better if you add the generaltable class and remove all the current CSS targetting this popup.

            Show
            bawjaws David Scotson added a comment - There's a few odd things here: 1) this should probably have the generaltable class, rather than generalbox. If it did, it would pick up the standard Bootstrap table styles with striping and generally look a little nicer. 2) the width of the Rating column header is set to be 100% (of the width of the table) which is obviously not correct and seems to create chaos under certain circumstances. I think this is what's causing the name to overlap with the image sometimes. 3) there's specific styles added to center align the data, but not the headers. But strangely, possibly due to number 2) above, all the columns but Rating look left-aligned. 4) there's a no-wrap style set, which if you remove causes the table to become a mess, but again this seems to be a symptom of 2) above, once you remove that the table adapts to small screens without too much fuss So to me it looks better if you add the generaltable class and remove all the current CSS targetting this popup.
            Hide
            tsala Helen Foster added a comment -

            David, thanks a lot for your comments and suggested fix. If I could rate it as useful I would have done so immediately.

            Show
            tsala Helen Foster added a comment - David, thanks a lot for your comments and suggested fix. If I could rate it as useful I would have done so immediately.
            Hide
            barbararamiro Barbara Ramiro added a comment -

            Thanks David Scotson for the patch. David Mudrak, do you want to pull David's patch?

            Cheers (" ,)

            Show
            barbararamiro Barbara Ramiro added a comment - Thanks David Scotson for the patch. David Mudrak , do you want to pull David's patch? Cheers (" ,)
            Hide
            mudrd8mz David Mudrak added a comment -

            Thanks David for the patch. Two things.

            1. Firstly, I believe this should be probably fixed upstream in Moodle itself for Moodle 2.7 (and eventually backported to stable branches but that might cause some regressions in styling). To be honest, I am generally pretty resistant to patching moodle.org unless there are really good reasons. If this can wait couple of months yet and David can get his solution upstream, I would personally prefer that.

            2. David, in either case, please do not mix-in irrelevant changes into one patch. Tiding up comments and coding style (such as adding space bar after comma or dots to the end of a sentence) are good to have. But please, isolate them into a separate patch at your branch. Having them all in one patch makes it less obvious what the change is really about and also makes eventual backporting/hot fixing more difficult (more modified lines = bigger chance for a conflict). What used to work for me in the past (as I also could not help myself and improved these things while being in a particular area) was to "git add -p" individual chunks separately. Then the branch consists of separate, well focused and described commits. It is both easier to backport as well as review during the integration review.

            Show
            mudrd8mz David Mudrak added a comment - Thanks David for the patch. Two things. 1. Firstly, I believe this should be probably fixed upstream in Moodle itself for Moodle 2.7 (and eventually backported to stable branches but that might cause some regressions in styling). To be honest, I am generally pretty resistant to patching moodle.org unless there are really good reasons. If this can wait couple of months yet and David can get his solution upstream, I would personally prefer that. 2. David, in either case, please do not mix-in irrelevant changes into one patch. Tiding up comments and coding style (such as adding space bar after comma or dots to the end of a sentence) are good to have. But please, isolate them into a separate patch at your branch. Having them all in one patch makes it less obvious what the change is really about and also makes eventual backporting/hot fixing more difficult (more modified lines = bigger chance for a conflict). What used to work for me in the past (as I also could not help myself and improved these things while being in a particular area) was to "git add -p" individual chunks separately. Then the branch consists of separate, well focused and described commits. It is both easier to backport as well as review during the integration review.
            Hide
            bawjaws David Scotson added a comment -

            Hi David,

            1. totally agree, in fact I had assumed that would be the case

            Also, good point about 2., my editor runs the Moodle style checker every time I save so I get bugged every time I save a file with these types of errors until I cave and fix them, and this file seemed tantalizingly close to passing, but yes I should have separated them out into separate commits.

            I've now done so. I'd actually forgotten that I'd changed the error text when no ratings present to use the notification API, I'm not sure if that needs a bug all to itself, I've done it as a seperate commit for now.

            Show
            bawjaws David Scotson added a comment - Hi David, 1. totally agree, in fact I had assumed that would be the case Also, good point about 2., my editor runs the Moodle style checker every time I save so I get bugged every time I save a file with these types of errors until I cave and fix them, and this file seemed tantalizingly close to passing, but yes I should have separated them out into separate commits. I've now done so. I'd actually forgotten that I'd changed the error text when no ratings present to use the notification API, I'm not sure if that needs a bug all to itself, I've done it as a seperate commit for now.
            Hide
            barbararamiro Barbara Ramiro added a comment -

            Hi David Mudrak, while it is not in core yet with David Scotson's patch, here goes for the theme plus a bit of styling. https://github.com/moodlehq/moodle-theme_moodleofficial/pull/31

            Cheers (" ,)

            Show
            barbararamiro Barbara Ramiro added a comment - Hi David Mudrak , while it is not in core yet with David Scotson 's patch, here goes for the theme plus a bit of styling. https://github.com/moodlehq/moodle-theme_moodleofficial/pull/31 Cheers (" ,)
            Hide
            mudrd8mz David Mudrak added a comment -

            Merged to the theme, thanks.

            Show
            mudrd8mz David Mudrak added a comment - Merged to the theme, thanks.
            Hide
            mudrd8mz David Mudrak added a comment -

            This has been deployed to the next.moodle.org site for testing of the upcoming -r6 live site revision. It will help if you can try and test this issue at the next.moodle.org site and re-open it if you find some problems. Thanks in advance.

            Show
            mudrd8mz David Mudrak added a comment - This has been deployed to the next.moodle.org site for testing of the upcoming -r6 live site revision. It will help if you can try and test this issue at the next.moodle.org site and re-open it if you find some problems. Thanks in advance.
            Hide
            tsala Helen Foster added a comment -

            Looks fine now, thanks Barbara, David S and David M

            Show
            tsala Helen Foster added a comment - Looks fine now, thanks Barbara, David S and David M

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Development