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

          Attachments

            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