Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.7
    • Fix Version/s: 2.7
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. This must be tested with all themes.
      2. For each theme go to the following location, you should see a tabular data. Make sure it looks identical to the one without this patch. The thing to verify mostly is alternate colors for tr background.
        1. User enrollement table
        2. Live logs Reports for the site
        3. Logs Reports for the site
        4. admin/tool/customlang/index.php (open a pack for editing)
        5. Blocks list (admin/blocks.php)
        6. Question bank listing all questions
        7. Create a survey with type "COLLES(ACTUAL)" and attempt it as student
      Show
      This must be tested with all themes. For each theme go to the following location, you should see a tabular data. Make sure it looks identical to the one without this patch. The thing to verify mostly is alternate colors for tr background. User enrollement table Live logs Reports for the site Logs Reports for the site admin/tool/customlang/index.php (open a pack for editing) Blocks list (admin/blocks.php) Question bank listing all questions Create a survey with type "COLLES(ACTUAL)" and attempt it as student
    • Affected Branches:
      MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull Master Branch:
      MDL-43804-master
    • Sprint:
      BACKEND Sprint 10
    • Sprint:
      BACKEND Sprint 10

      Description

      Remove r0, r1 specific css from themes.

      table.flexible .r0, table.generaltable .r0 {background-color: #F0F0F0;}
      table.flexible .r1, table.generaltable .r1 {background-color: #FAFAFA;}
      

      This creates problem when adding new rows dynamically to the table.

      We should use instead nth-child(even/odd).

      It is supported by all browsers supported by Moodle:

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Pushing this for peer-review

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Pushing this for peer-review Thanks
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43804

            • Remote repository: git://github.com/ankitagarwal/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43804 Remote repository: git://github.com/ankitagarwal/moodle.git Remote branch MDL-43804 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/749 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/749/artifact/work/smurf.html
            Hide
            dobedobedoh Andrew Nicols added a comment -

            This looks good to me and makes sense.

            Just to clarify for anyone looking at this issue later, without making this change, if we want to dynamically add rows to a table we will have to add an offset to get the correct row numbering (odd/even). This is a much safer, and more reliable solution and is compatible with all supported browsers.

            I think that this should be good to go for integration. I've been having an internal debate as to whether it would be wise to keep the r0/r1 classes in place for non-core themes - but I don't think that this is necessary or wise - it will not be spotted until something weird starts to happen in a dynamic table, and will be difficult to diagnose.

            Show
            dobedobedoh Andrew Nicols added a comment - This looks good to me and makes sense. Just to clarify for anyone looking at this issue later, without making this change, if we want to dynamically add rows to a table we will have to add an offset to get the correct row numbering (odd/even). This is a much safer, and more reliable solution and is compatible with all supported browsers. I think that this should be good to go for integration. I've been having an internal debate as to whether it would be wise to keep the r0/r1 classes in place for non-core themes - but I don't think that this is necessary or wise - it will not be spotted until something weird starts to happen in a dynamic table, and will be difficult to diagnose.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Andrew,

            I don't think we should remove those classes either, for non-core themes.
            Pushing for integration.

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Andrew, I don't think we should remove those classes either, for non-core themes. Pushing for integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Hi, some comments:

            1) Should we "kill" the r0/r1 generation at some point? Please discuss about that. If agreed this needs some // TODO or @todo, pointing to a new issue, and also upgrade.txt should point about when r0/r1 are about to disappear.

            2) Curiously, just a few days ago I was reading about nth-child and friends and it was clear that using nth-of-type was far better for uses like this. Note it has no much importance in this case (they are "tr" within a "table"), but to do it 100% correctly, I think we should be using nth-of-type instead (imagine we add head/body tags, "nth-child" will lead to odd/even flip-flopping. nth-of-type won't). Reference:

            http://css-tricks.com/the-difference-between-nth-child-and-nth-of-type/

            So, mainly because of 1) we need to, always, define the TTL of any "deprecated" functionality, but also coz 2)... reopening this for another round/discussion. Thanks!

            Edited, to add link to the page about nth diffs.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Hi, some comments: 1) Should we "kill" the r0/r1 generation at some point? Please discuss about that. If agreed this needs some // TODO or @todo, pointing to a new issue, and also upgrade.txt should point about when r0/r1 are about to disappear. 2) Curiously, just a few days ago I was reading about nth-child and friends and it was clear that using nth-of-type was far better for uses like this. Note it has no much importance in this case (they are "tr" within a "table"), but to do it 100% correctly, I think we should be using nth-of-type instead (imagine we add head/body tags, "nth-child" will lead to odd/even flip-flopping. nth-of-type won't). Reference: http://css-tricks.com/the-difference-between-nth-child-and-nth-of-type/ So, mainly because of 1) we need to, always, define the TTL of any "deprecated" functionality, but also coz 2)... reopening this for another round/discussion. Thanks! Edited, to add link to the page about nth diffs.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Discussed this with Andrew again, and we agree that we can remove this in Moodle 2.9. This method of using r0/r1 for defining CSS for alternate child is highly unreliable and should be avoided. I will try to get a few more views on this and see if there are any major objection.

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Discussed this with Andrew again, and we agree that we can remove this in Moodle 2.9. This method of using r0/r1 for defining CSS for alternate child is highly unreliable and should be avoided. I will try to get a few more views on this and see if there are any major objection.
            Hide
            timhunt Tim Hunt added a comment -

            Before this is integrated, you should post in the Themes forum and General developer forum.

            (Actually, you should have done that before now, rather that just plotting this in private, but better late than never.)

            Theme/upgrade.txt could be better:

            • You could give exact instructions for how to upgrade your .r0 and .r1 rules to the new way.
            • Rather than describing this as a 'recommendation' you could point out that this change is necessary, because the r0 and r1 classes will be removed in Moodle 2.x
            • You could upgrade codechecker to check .css files and point out uses of .r0 and .r1 classes.

            Please remember that every API change makes lots of work for all add-on developers.

            Show
            timhunt Tim Hunt added a comment - Before this is integrated, you should post in the Themes forum and General developer forum. (Actually, you should have done that before now, rather that just plotting this in private, but better late than never.) Theme/upgrade.txt could be better: You could give exact instructions for how to upgrade your .r0 and .r1 rules to the new way. Rather than describing this as a 'recommendation' you could point out that this change is necessary, because the r0 and r1 classes will be removed in Moodle 2.x You could upgrade codechecker to check .css files and point out uses of .r0 and .r1 classes. Please remember that every API change makes lots of work for all add-on developers.
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Thanks Tim for the feedback.

            I have created a forum post to discuss this further (https://moodle.org/mod/forum/discuss.php?d=252996) (There is no plotting going on here, this was raised in the dev chat, which is always the first step to get more feedback)

            About Theme/upgrade.txt

            1. Sure, why not.
            2. It is a 'recommendation' at this point. Eloy suggested to remove the classes in future and me and Andrew agreed with it. We have not yet decided to go in that direction, once we do, the upgrade.txt will be updated.
            3. Sure. Our code checker doesn't parse CSS files at the moment, afaik. Anyway I can create an MDLSITE for this.

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Thanks Tim for the feedback. I have created a forum post to discuss this further ( https://moodle.org/mod/forum/discuss.php?d=252996 ) (There is no plotting going on here, this was raised in the dev chat, which is always the first step to get more feedback) About Theme/upgrade.txt Sure, why not. It is a 'recommendation' at this point. Eloy suggested to remove the classes in future and me and Andrew agreed with it. We have not yet decided to go in that direction, once we do, the upgrade.txt will be updated. Sure. Our code checker doesn't parse CSS files at the moment, afaik. Anyway I can create an MDLSITE for this. Thanks
            Hide
            bawjaws David Scotson added a comment -

            I filed a similar bug a while back (MDL-40237).

            As noted in that bug Moodle also uses the classes .odd and .even for this same thing.

            It's also always struck me as odd (no pun intended) that the first row (and all other odd-numbered rows) got the number 0, and the second row (and all other even numbered rows) got the number 1.

            I don't think the Bootstrapbase or Clean themes currently use these classes at all so the change shouldn't affect them too much. Actually I just checked and someone added them for the survey module late last year (https://github.com/moodle/moodle/commit/1765866fff644d1652adde38d151377d87bafe5d) rather than re-use the table-striped class that stripes the rest of the tables in the theme.

            Also, is it just my imagination or are people going out of their way to not mention that this doesn't work in IE8?

            Show
            bawjaws David Scotson added a comment - I filed a similar bug a while back ( MDL-40237 ). As noted in that bug Moodle also uses the classes .odd and .even for this same thing. It's also always struck me as odd (no pun intended) that the first row (and all other odd-numbered rows) got the number 0, and the second row (and all other even numbered rows) got the number 1. I don't think the Bootstrapbase or Clean themes currently use these classes at all so the change shouldn't affect them too much. Actually I just checked and someone added them for the survey module late last year ( https://github.com/moodle/moodle/commit/1765866fff644d1652adde38d151377d87bafe5d ) rather than re-use the table-striped class that stripes the rest of the tables in the theme. Also, is it just my imagination or are people going out of their way to not mention that this doesn't work in IE8?
            Hide
            bawjaws David Scotson added a comment -

            Looking back at the stuff I did to get striped tables with Bootstrap:

            https://github.com/moodle/moodle/blob/master/theme/bootstrapbase/less/moodle/expendable.less#L1-L14

            I note that apparently only targetting the table.flexible and .generaltable classes didn't seem to be enough, so you may want to go through that list of tables:

            table#explaincaps,
            table#defineroletable,
            table.grading-report,
            table#listdirectories,
            table.rolecaps,
            table.userenrolment,
            table#form,
            form#movecourses table,
            #page-admin-course-index .editcourse,
            .forumheaderlist,

            and add one of the two "standard" table classes, to ensure they get striped properly (MDL-40497 suggests that these two classnames should really be collapsed into a single one, and suggests .table).

            Show
            bawjaws David Scotson added a comment - Looking back at the stuff I did to get striped tables with Bootstrap: https://github.com/moodle/moodle/blob/master/theme/bootstrapbase/less/moodle/expendable.less#L1-L14 I note that apparently only targetting the table.flexible and .generaltable classes didn't seem to be enough, so you may want to go through that list of tables: table#explaincaps, table#defineroletable, table.grading-report, table#listdirectories, table.rolecaps, table.userenrolment, table#form, form#movecourses table, #page-admin-course-index .editcourse, .forumheaderlist, and add one of the two "standard" table classes, to ensure they get striped properly ( MDL-40497 suggests that these two classnames should really be collapsed into a single one, and suggests .table).
            Hide
            gb2048 Gareth J Barnard added a comment -

            Does IE8 matter so much in M2.6+ given -> http://docs.moodle.org/dev/Moodle_2.6_release_notes#Requirements. Surely we have to move on and improve and not be saddled to the constraints of the past. Otherwise we would still have to support Netscape Navigator and people would be asking for a Windows 286 version encapsulated within a virtual browser.

            Show
            gb2048 Gareth J Barnard added a comment - Does IE8 matter so much in M2.6+ given -> http://docs.moodle.org/dev/Moodle_2.6_release_notes#Requirements . Surely we have to move on and improve and not be saddled to the constraints of the past. Otherwise we would still have to support Netscape Navigator and people would be asking for a Windows 286 version encapsulated within a virtual browser.
            Hide
            bawjaws David Scotson added a comment -

            Regarding Eloy's comment about nth-of-type, the correct solution looks more like this:

            https://github.com/twbs/bootstrap/blob/master/less/tables.less#L107-L114

            i.e. only add the striping to the rows within the tbody element.

            Which reminds me, another small issue that this uncovers is the use of .generaltable for things that are neither general nor strictly tables, e.g. the layout for the assign roles screens are laid out via a table with the class .generaltable, so the current patch will probably give that layout table a pale gray background. There's more info in MDL-41361.

            Show
            bawjaws David Scotson added a comment - Regarding Eloy's comment about nth-of-type, the correct solution looks more like this: https://github.com/twbs/bootstrap/blob/master/less/tables.less#L107-L114 i.e. only add the striping to the rows within the tbody element. Which reminds me, another small issue that this uncovers is the use of .generaltable for things that are neither general nor strictly tables, e.g. the layout for the assign roles screens are laid out via a table with the class .generaltable, so the current patch will probably give that layout table a pale gray background. There's more info in MDL-41361 .
            Hide
            dwahl Daniel Wahl added a comment -

            I just posted this in the forum but I think it's important enough to post it here too, looking at the diff I think we could do this much easier by lowering the specificity on our selector (sorry for copy pasta from forum):

            just for the sake of consideration (and ease on themers...) I think we could lower the specificity on those selectors too. Instead of:

            `table.generaltable tr:nth-of-type(odd)`

            and the 50 other rules in the diff, how about

            `.generaltable tr:nth-of-type(odd)`

            or

            `tr:nth-of-type(odd)`

            here's my reasoning:

            #lower specificity is ALWAYS better in CSS.
            #you'll never have a semantically valid tr outside of a table so you can never unintentionally target a tr
            #unfortunately not all tables in Moodle use the generaltable class - so you'll end up adding another rule or selector at some point (actually looking at the diff this is already happening.)

            also, do we want all tables to be striped or should we give the option to opt out with something like this:

            `tr.no-stripe

            {background:inherit;}

            `
            or
            `.no-stripe tr

            {background:inherit;}

            `
            which is probably easier for programmers to output and is less code

            Show
            dwahl Daniel Wahl added a comment - I just posted this in the forum but I think it's important enough to post it here too, looking at the diff I think we could do this much easier by lowering the specificity on our selector (sorry for copy pasta from forum): just for the sake of consideration (and ease on themers...) I think we could lower the specificity on those selectors too. Instead of: `table.generaltable tr:nth-of-type(odd)` and the 50 other rules in the diff, how about `.generaltable tr:nth-of-type(odd)` or `tr:nth-of-type(odd)` here's my reasoning: #lower specificity is ALWAYS better in CSS. #you'll never have a semantically valid tr outside of a table so you can never unintentionally target a tr #unfortunately not all tables in Moodle use the generaltable class - so you'll end up adding another rule or selector at some point (actually looking at the diff this is already happening.) also, do we want all tables to be striped or should we give the option to opt out with something like this: `tr.no-stripe {background:inherit;} ` or `.no-stripe tr {background:inherit;} ` which is probably easier for programmers to output and is less code
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks for the feedback.

            1. David, as Gareth mentioned IE8 is not supported any more , hence there is no need to worry about it.
            2. Also thanks for pointing out the issue in the forum, r0 should be odd and r1 should be even, not the other way around. I have updated the patch accordingly. Regarding, changing css to look for tr inside tbody, do we really need to go to that specific level?
            3. Daniel, I am not in favour of adding a CSS this generic, 'tr:nth-of-type(odd)' , since it is going to create a lot of work for not only theme developers but also other plugin developers who don't want their tables stripped.
            4. I have also created a deprecation todo for Moodle 2.9 and updated the readme.(MDL-43902)
            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks for the feedback. David, as Gareth mentioned IE8 is not supported any more , hence there is no need to worry about it. Also thanks for pointing out the issue in the forum, r0 should be odd and r1 should be even, not the other way around. I have updated the patch accordingly. Regarding, changing css to look for tr inside tbody, do we really need to go to that specific level? Daniel, I am not in favour of adding a CSS this generic, 'tr:nth-of-type(odd)' , since it is going to create a lot of work for not only theme developers but also other plugin developers who don't want their tables stripped. I have also created a deprecation todo for Moodle 2.9 and updated the readme.( MDL-43902 )
            Hide
            bawjaws David Scotson added a comment -

            Regarding #2, if you don't target within the tbody then you are targetting the table header row(s) too. So it might work most of the time, but it's not doing what most people would expect it to do, which is to stripe the data rows in the table, not just every row.

            It's also a change from the current behaviour where the header row doesn't get a r0 class added to it, but it will now be targetted by this rule.

            As an added benefit, the use of this rule in Bootstrap helped us identify several areas where people hadn't marked up the header row correctly, which is an accessibility problem. This way of doing it neatly guides people to do the right thing.

            It's also, as mentioned, the way that Bootstrap does it, so it simplifies people's lives if they don't have to worry about the subtle corner-cases in the way that their tables might display between different themes.

            Show
            bawjaws David Scotson added a comment - Regarding #2, if you don't target within the tbody then you are targetting the table header row(s) too. So it might work most of the time, but it's not doing what most people would expect it to do, which is to stripe the data rows in the table, not just every row. It's also a change from the current behaviour where the header row doesn't get a r0 class added to it, but it will now be targetted by this rule. As an added benefit, the use of this rule in Bootstrap helped us identify several areas where people hadn't marked up the header row correctly, which is an accessibility problem. This way of doing it neatly guides people to do the right thing. It's also, as mentioned, the way that Bootstrap does it, so it simplifies people's lives if they don't have to worry about the subtle corner-cases in the way that their tables might display between different themes.
            Hide
            bawjaws David Scotson added a comment -

            For reference, the CSS that Bootstrap uses is:

            .table-striped>tbody>tr:nth-child(odd)>td,
            .table-striped>tbody>tr:nth-child(odd)>th{
            background-color:#f9f9f9
            }

            Also, I compared the logs page in Standard and BootstrapBase/Clean. Standard has a .generaltable .cell class which overrides the stripes by re-applying a white background to all the cells. Anyone know if that's a mistake or not?

            As far as I can tell it's done here on the HTML side, and it's true for every table generated via html_writer::table.

            https://github.com/moodle/moodle/blob/master/lib/outputcomponents.php#L1615

            With this being the CSS that overules the stripes (but only for the Standard theme, not themes that inherit from Base):

            https://github.com/moodle/moodle/blob/master/theme/standard/style/core.css#L68

            Show
            bawjaws David Scotson added a comment - For reference, the CSS that Bootstrap uses is: .table-striped>tbody>tr:nth-child(odd)>td, .table-striped>tbody>tr:nth-child(odd)>th{ background-color:#f9f9f9 } Also, I compared the logs page in Standard and BootstrapBase/Clean. Standard has a .generaltable .cell class which overrides the stripes by re-applying a white background to all the cells. Anyone know if that's a mistake or not? As far as I can tell it's done here on the HTML side, and it's true for every table generated via html_writer::table. https://github.com/moodle/moodle/blob/master/lib/outputcomponents.php#L1615 With this being the CSS that overules the stripes (but only for the Standard theme, not themes that inherit from Base): https://github.com/moodle/moodle/blob/master/theme/standard/style/core.css#L68
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            patch updated, requesting review.

            Show
            ankit_frenz Ankit Agarwal added a comment - patch updated, requesting review.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            I have added the tbody, as suggested by David after discussing with Andrew and Fred, since all tables must have a tbody anyway and we don't want to strip the header rows.

            Show
            ankit_frenz Ankit Agarwal added a comment - I have added the tbody, as suggested by David after discussing with Andrew and Fred, since all tables must have a tbody anyway and we don't want to strip the header rows.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Cheers Ankit,

            So I agree that the best thing to do here is to add tbody into the mix.
            I think that the distinction between nth-child and nth-of-type should be moot here. Technically, a tbody can contain tr tags and other script-supporting elements (http://www.w3.org/TR/html5/tabular-data.html#the-tbody-element) so it doesn't make a huge amount of difference.

            My only other thought would be to add the new selectors without removing the old ones. I say this because it although we no longer support IE8 I don't think we should deliberately remove support for it yet. The new selector is more specific so should take precedence, and the only place in which it will appear squiffy will be the new dynamically update tables.

            If we add the old selectors back in such a way that they're above the new ones on their own line, we can remove them in 2.9 with the minimum of history loss:

            .que .r0,
            .que > tbody > tr:nth-of-type(odd) {background-color: #F5F5F5;}
            .que .r1,
            .que > tbody > tr:nth-of-type(even) {background-color: #EEE;}
            

            It may be worth waiting for other's views before making the above change and checking with Integrators too.

            Otherwise, this issue looks good to go for integration IMO.

            Cheers, and welcome to the frontend team

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Cheers Ankit, So I agree that the best thing to do here is to add tbody into the mix. I think that the distinction between nth-child and nth-of-type should be moot here. Technically, a tbody can contain tr tags and other script-supporting elements ( http://www.w3.org/TR/html5/tabular-data.html#the-tbody-element ) so it doesn't make a huge amount of difference. My only other thought would be to add the new selectors without removing the old ones. I say this because it although we no longer support IE8 I don't think we should deliberately remove support for it yet. The new selector is more specific so should take precedence, and the only place in which it will appear squiffy will be the new dynamically update tables. If we add the old selectors back in such a way that they're above the new ones on their own line, we can remove them in 2.9 with the minimum of history loss: .que .r0, .que > tbody > tr:nth-of-type(odd) {background-color: #F5F5F5;} .que .r1, .que > tbody > tr:nth-of-type(even) {background-color: #EEE;} It may be worth waiting for other's views before making the above change and checking with Integrators too. Otherwise, this issue looks good to go for integration IMO. Cheers, and welcome to the frontend team Andrew
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Andrew for the review.

            I don't really like the idea of keeping the old selector. Basically because:-

            1. Ie8 is ancient, we should not keep code, just for browsers that we don't support anymore.
            2. Removing both css and the class from html suddenly in 2.9, doesn't sound as per our deprecation policy.

            Having said that, I donot have any major objections keeping these selectors in place, if integrators agree with it.

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Andrew for the review. I don't really like the idea of keeping the old selector. Basically because:- Ie8 is ancient, we should not keep code, just for browsers that we don't support anymore. Removing both css and the class from html suddenly in 2.9, doesn't sound as per our deprecation policy. Having said that, I donot have any major objections keeping these selectors in place, if integrators agree with it.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I'd vote for keeping the rc0 and rc1 being generated till 2.9, but cleaning any use from core, and get rid of their generation in 2.9 (standard deprecation + annihilation process).

            So, under those premises, I think your current patch looks perfect:

            1) r0, r1 continue being generated.
            2) No r0, r1 uses in core css (all them replaced by the new nth ones).
            3) Correct upgrade.txt message (perhaps it could include a link to the MDL-43902).
            4) There is a task to implement the check by the code-checker.

            +1 Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I'd vote for keeping the rc0 and rc1 being generated till 2.9, but cleaning any use from core, and get rid of their generation in 2.9 (standard deprecation + annihilation process). So, under those premises, I think your current patch looks perfect: 1) r0, r1 continue being generated. 2) No r0, r1 uses in core css (all them replaced by the new nth ones). 3) Correct upgrade.txt message (perhaps it could include a link to the MDL-43902 ). 4) There is a task to implement the check by the code-checker. +1 Ciao
            Hide
            bawjaws David Scotson added a comment -

            You could add the .ie8 class before the rules that are intended for IE8 only.

            This a) completely stops them affecting other browsers (as there might be some interference depending on how the stripes are defined, sometimes you only specify color for one or the other and removing a row via javascript would mean that all the odds after that become even and vice versa, but r0 and r1 will remain attached to the same row), b) documents what's going on a bit better, c) encourages developers (who will not be using IE8) to make the shift to the new way of doing things and d) makes these rules easy to find and remove at a later date,

            (I already have script that can strip .ie7 and .ie6 specific code that's coded in this way, which helps save some space, and lets people who know they don't need the support get a head start on the benefits)

            Show
            bawjaws David Scotson added a comment - You could add the .ie8 class before the rules that are intended for IE8 only. This a) completely stops them affecting other browsers (as there might be some interference depending on how the stripes are defined, sometimes you only specify color for one or the other and removing a row via javascript would mean that all the odds after that become even and vice versa, but r0 and r1 will remain attached to the same row), b) documents what's going on a bit better, c) encourages developers (who will not be using IE8) to make the shift to the new way of doing things and d) makes these rules easy to find and remove at a later date, (I already have script that can strip .ie7 and .ie6 specific code that's coded in this way, which helps save some space, and lets people who know they don't need the support get a head start on the benefits)
            Hide
            cibot CiBoT added a comment -

            Results for MDL-43804

            • Remote repository: git://github.com/ankitagarwal/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-43804 Remote repository: git://github.com/ankitagarwal/moodle.git Remote branch MDL-43804 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1116 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1116/artifact/work/smurf.html
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks for all the feedback on this, really appreciated.

            I am submitting this for integration as I do not think we should be considering backward compatibility for unsupported versions, while writing the improvements. Also this can go forward now, since an integrator as +1ed it.

            Cheers

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks for all the feedback on this, really appreciated. I am submitting this for integration as I do not think we should be considering backward compatibility for unsupported versions, while writing the improvements. Also this can go forward now, since an integrator as +1ed it. Cheers
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Ankit this has been integrated now.

            Please note I did add a commit to add a little more detail to theme/upgrade.txt (pre-empting the "why" questions)

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Ankit this has been integrated now. Please note I did add a commit to add a little more detail to theme/upgrade.txt (pre-empting the "why" questions)
            Hide
            bawjaws David Scotson added a comment -

            Just to note that on Survey, at least one of the Survey question types (mis-)used the r0 and r1 classes to highlight the rows two at a time.

            I think it works better with alternating rows, but thought I'd mention it anyway.

            Show
            bawjaws David Scotson added a comment - Just to note that on Survey, at least one of the Survey question types (mis-)used the r0 and r1 classes to highlight the rows two at a time. I think it works better with alternating rows, but thought I'd mention it anyway.
            Hide
            damyon Damyon Wiese added a comment -

            What a fun test.

            Passed.

            Thanks!

            Show
            damyon Damyon Wiese added a comment - What a fun test. Passed. Thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Fetch your remotes, prune them,
            clean your integrated branches and say "Ahem".

            Rebase your ongoing stuff, keep conflicts away
            don't forget to test the code and we'll love you again.

            Thanks, closing!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Fetch your remotes, prune them, clean your integrated branches and say "Ahem". Rebase your ongoing stuff, keep conflicts away don't forget to test the code and we'll love you again. Thanks, closing!

              People

              • Votes:
                5 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14

                  Agile