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

Hi Mary,

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

@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.

@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.

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

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

@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.

@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.

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.

@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.

@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.

@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.

@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.

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

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

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.

Many, many thanks for your effort!

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

Closing as fixed. Ciao