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

Course categories view /course/index.php, apply .generaltable class instead of .generalbox

    Details

      Description

      Course categories view /course/index.php contains a table with classname generalbox. This gives rounded corners to the contents of the table (even though the table corners are not given the same border-radius). Looks strange and is an obscure enough page view that many theme designers won't catch it to override. Class should be generaltable instead.

      Replicate:

      1. Navigate to ../course/index.php?categoryedit=on
      2. Observe table in center of page (note rounded corners on background color inside of square-cornered table)

      Test:

      1. Apply patch
      2. Navigate to ../course/index.php?categoryedit=on
      3. Observe table in center of page (note there are no longer rounded corners, table is styled according to .generaltable CSS)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            Wow, yes, rounded corners on bottom corners if you look carefully.

            Show
            poltawski Dan Poltawski added a comment - Wow, yes, rounded corners on bottom corners if you look carefully.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Mary,

            I was wondering if you could review this change (any implications on our other themes)

            Show
            poltawski Dan Poltawski added a comment - Hi Mary, I was wondering if you could review this change (any implications on our other themes)
            Hide
            lazydaisy Mary Evans added a comment - - edited

            @Dan

            Yes I can, but on the face of it and looking at the code I don't foresee any problem. However I am curious to know how the table background goes from grey to white, when there is no CSS in the patch. Or did I miss something.

            Post Script:
            I think it is an optical illusion comparing the two images, one looks darker than the other. I'll check the themes later today.

            Show
            lazydaisy Mary Evans added a comment - - edited @Dan Yes I can, but on the face of it and looking at the code I don't foresee any problem. However I am curious to know how the table background goes from grey to white, when there is no CSS in the patch. Or did I miss something. Post Script: I think it is an optical illusion comparing the two images, one looks darker than the other. I'll check the themes later today.
            Hide
            agroshek Amy Groshek added a comment -

            @Mary

            It's not an optical illusion. The patch changes a CSS selector. The table is being given the .generalbox classname (which is meant for boxes, not tables). .generalbox gives the table that prodigious 15px border-radius on the two bottom corners, and a dark grey background. The fix changes the classname to .generaltable, which was originally laid down for tables.

            Show
            agroshek Amy Groshek added a comment - @Mary It's not an optical illusion. The patch changes a CSS selector. The table is being given the .generalbox classname (which is meant for boxes, not tables). .generalbox gives the table that prodigious 15px border-radius on the two bottom corners, and a dark grey background. The fix changes the classname to .generaltable, which was originally laid down for tables.
            Hide
            lazydaisy Mary Evans added a comment -

            Of course it does...thanks for the reminder.

            Show
            lazydaisy Mary Evans added a comment - Of course it does...thanks for the reminder.
            Hide
            tbannister Tyler Bannister added a comment - - edited

            I thought I'd lend a hand for the peer review:

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

            The git check failed because:

            1. The commit message doesn't explain why the change was needed. The reason for the change (fixing display issue) is more important than describing the actual change (changing table class name).

            The sanity check failed because:
            There's a similar problem on some other pages (which may not be visible if the table color is the same as the page color):

            1. admin/mnet/index.php (2 instances)
            2. admin/tool/xmldb/actions/check_bigints/check_bigints.class.php
            3. admin/tool/xmldb/actions/check_defaults/check_defaults.class.php
            4. admin/tool/xmldb/actions/check_foreign_keys/check_foreign_keys.class.php
            5. admin/tool/xmldb/actions/check_indexes/check_indexes.class.php
            6. admin/tool/xmldb/actions/check_oracle_semantics/check_oracle_semantics.class.php
            7. admin/tool/xmldb/actions/delete_field/delete_field.class.php
            8. admin/tool/xmldb/actions/delete_index/delete_index.class.php
            9. admin/tool/xmldb/actions/delete_table/delete_table.class.php
            10. admin/tool/xmldb/actions/delete_xml_file/delete_xml_file.class.php
            11. admin/tool/xmldb/actions/revert_changes/revert_changes.class.php
            12. admin/tool/xmldb/actions/XMLDBCheckAction.class.php
            13. blocks/completionstatus/details.php (2 instances)
            14. course/category.php (2 instances)
            15. course/lib.php
            16. course/search.php
            17. mod/feedback/analysis.php
            18. mod/feedback/analysis_course.php
            19. mod/glossary/editcategories.html
            20. mod/glossary/editcategories.php
            21. mod/glossary/formats.php
            22. mod/wiki/pagelib.php (5 instances)
            23. report/completion/user.php
            Show
            tbannister Tyler Bannister added a comment - - edited I thought I'd lend a hand for the peer review: [-] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [N] Git [N] Sanity check The git check failed because: The commit message doesn't explain why the change was needed. The reason for the change (fixing display issue) is more important than describing the actual change (changing table class name). The sanity check failed because: There's a similar problem on some other pages (which may not be visible if the table color is the same as the page color): admin/mnet/index.php (2 instances) admin/tool/xmldb/actions/check_bigints/check_bigints.class.php admin/tool/xmldb/actions/check_defaults/check_defaults.class.php admin/tool/xmldb/actions/check_foreign_keys/check_foreign_keys.class.php admin/tool/xmldb/actions/check_indexes/check_indexes.class.php admin/tool/xmldb/actions/check_oracle_semantics/check_oracle_semantics.class.php admin/tool/xmldb/actions/delete_field/delete_field.class.php admin/tool/xmldb/actions/delete_index/delete_index.class.php admin/tool/xmldb/actions/delete_table/delete_table.class.php admin/tool/xmldb/actions/delete_xml_file/delete_xml_file.class.php admin/tool/xmldb/actions/revert_changes/revert_changes.class.php admin/tool/xmldb/actions/XMLDBCheckAction.class.php blocks/completionstatus/details.php (2 instances) course/category.php (2 instances) course/lib.php course/search.php mod/feedback/analysis.php mod/feedback/analysis_course.php mod/glossary/editcategories.html mod/glossary/editcategories.php mod/glossary/formats.php mod/wiki/pagelib.php (5 instances) report/completion/user.php
            Hide
            lazydaisy Mary Evans added a comment - - edited

            @Tyler
            umm...

            @Amy
            Since this is only going to affect Standard theme, where the CSS3 border radius is set, it is hardly likely to affect CORE themes as there is minimal CSS in Base theme for both .generalbox and .generaltable anyway.

            I'm testing it now...so will report back shortly.

            Show
            lazydaisy Mary Evans added a comment - - edited @Tyler umm... @Amy Since this is only going to affect Standard theme, where the CSS3 border radius is set, it is hardly likely to affect CORE themes as there is minimal CSS in Base theme for both .generalbox and .generaltable anyway. I'm testing it now...so will report back shortly.
            Hide
            agroshek Amy Groshek added a comment -

            @Mary

            You are right that the other core themes don't use those classes, but this isn't only about what standard theme does right now. If we want to make theming easier, and faster, we need to standardize the use of classnames. This ticket illustrates why that consistency is important. Here we have a tweaky issue in a handful of remote page views because two CSS concepts have been combined in an unexpected way.

            Ideally, we have a standard for the use of .generalbox, and when I write css for .generalbox, I'm assuming it's going to be applied to divs that are content or text containers. Similarly, if I write css for .generaltable, then I'm assuming that's going to be for all tables on the site, unless the mod developer has a very good reason to force me to spend extra time on their mod. Classnames for tables are for tables; classnames for boxes are for boxes. We shouldn't be offering up markup to the page that mixes and matches them.

            IMHO, also, standard should be a reliable, multipurpose theme which theme developers can use as a parent for a child theme with minimal changes. If that's something we want as a community, that's another reason to make these changes. I wasn't aware that standard is not considered a core theme. That seems strange, as it is the default theme on all standard Moodle installs.

            Show
            agroshek Amy Groshek added a comment - @Mary You are right that the other core themes don't use those classes, but this isn't only about what standard theme does right now. If we want to make theming easier, and faster, we need to standardize the use of classnames. This ticket illustrates why that consistency is important. Here we have a tweaky issue in a handful of remote page views because two CSS concepts have been combined in an unexpected way. Ideally, we have a standard for the use of .generalbox, and when I write css for .generalbox, I'm assuming it's going to be applied to divs that are content or text containers. Similarly, if I write css for .generaltable, then I'm assuming that's going to be for all tables on the site, unless the mod developer has a very good reason to force me to spend extra time on their mod. Classnames for tables are for tables; classnames for boxes are for boxes. We shouldn't be offering up markup to the page that mixes and matches them. IMHO, also, standard should be a reliable, multipurpose theme which theme developers can use as a parent for a child theme with minimal changes. If that's something we want as a community, that's another reason to make these changes. I wasn't aware that standard is not considered a core theme. That seems strange, as it is the default theme on all standard Moodle installs.
            Hide
            lazydaisy Mary Evans added a comment -

            It all looks good to me. Nothing looked out of place apart from Afterburner which had no CSS styles for either generalbox or generaltable, so that needs fixing. Splash theme actually looks better with the patch than without! Which has to be a bonus.

            Show
            lazydaisy Mary Evans added a comment - It all looks good to me. Nothing looked out of place apart from Afterburner which had no CSS styles for either generalbox or generaltable, so that needs fixing. Splash theme actually looks better with the patch than without! Which has to be a bonus.
            Hide
            lazydaisy Mary Evans added a comment -

            @Amy

            Further to my last comment, if you want me to do more testing in the light of what Tyler has said, I'll happily test some more, but I will need specific pages to look for.

            Show
            lazydaisy Mary Evans added a comment - @Amy Further to my last comment, if you want me to do more testing in the light of what Tyler has said, I'll happily test some more, but I will need specific pages to look for.
            Hide
            agroshek Amy Groshek added a comment -

            @Mary

            Thanks for looking at this. I appreciate it. It looks like Tyler wants this issue to be resolved in all the other files where the same thing occurs before I can get a passing peer review. Which will be a much, much larger task.

            Show
            agroshek Amy Groshek added a comment - @Mary Thanks for looking at this. I appreciate it. It looks like Tyler wants this issue to be resolved in all the other files where the same thing occurs before I can get a passing peer review. Which will be a much, much larger task.
            Hide
            agroshek Amy Groshek added a comment -

            @Mary

            I'll see if I can find some time to track down all these other page views. To cover all of them the test instructions would be very extensive. For the reasons I put down above it might be worth it, though it might be, as one of my colleagues suggested this morning, "a can of worms." Will keep everyone posted here.

            Show
            agroshek Amy Groshek added a comment - @Mary I'll see if I can find some time to track down all these other page views. To cover all of them the test instructions would be very extensive. For the reasons I put down above it might be worth it, though it might be, as one of my colleagues suggested this morning, "a can of worms." Will keep everyone posted here.
            Hide
            lazydaisy Mary Evans added a comment -

            @Amy
            Sorry I missed your last comment, which I have just read. What I meant to have written was that the change re the border-radius was not going to affect OTHER core themes as they do not take their CSS from Standard theme.

            Also I do understand the need to get things standardized in Moodle and I am very pleased to see that a start has been made.

            Show
            lazydaisy Mary Evans added a comment - @Amy Sorry I missed your last comment, which I have just read. What I meant to have written was that the change re the border-radius was not going to affect OTHER core themes as they do not take their CSS from Standard theme. Also I do understand the need to get things standardized in Moodle and I am very pleased to see that a start has been made.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Get this set for integration, whatever issues might occur will be minimal, and can be dealt with when they show up, if anyone notices.

            Show
            lazydaisy Mary Evans added a comment - - edited Get this set for integration, whatever issues might occur will be minimal, and can be dealt with when they show up, if anyone notices.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Amy, i've integrated this to 23 and master

            Show
            poltawski Dan Poltawski added a comment - Thanks Amy, i've integrated this to 23 and master
            Hide
            abgreeve Adrian Greeve added a comment -

            Tested on the 2.3 and master integration branches.
            The table in course/index.php?categoryedit=on doesn't have rounded edges any more.
            No other problems found.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Tested on the 2.3 and master integration branches. The table in course/index.php?categoryedit=on doesn't have rounded edges any more. No other problems found. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Many, many thanks for your effort!

            Millions of people will enjoy the results of your work, yay!

            Closing as fixed. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jan/13