Moodle
  1. Moodle
  2. MDL-36576

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

    Details

    • Rank:
      46053

      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)

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

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

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

          Hi Mary,

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

          Show
          Dan Poltawski added a comment - Hi Mary, I was wondering if you could review this change (any implications on our other themes)
          Hide
          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
          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
          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
          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
          Mary Evans added a comment -

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

          Show
          Mary Evans added a comment - Of course it does...thanks for the reminder.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Thanks Amy, i've integrated this to 23 and master
          Hide
          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
          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
          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
          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: