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

Colours alternation is missing from tables in formal_white (and in most of other themes)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.4, 2.2.1, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Themes
    • Labels:
      None

      Description

      Table's rows should have alternate background colors for accessibility reasons.
      Formal white is missing them as it can easily seen in the user account list.
      As usual, I attach the code to patch formal_white code but, IMHO, this patch should be applied at central level (base or canvas) and not in a peripheral theme.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            daniss Daniele Cordella added a comment -

            I strongly believe this issue is a general theme issue and not restricted to formal_white only. Please consider to apply it at central level.

            Show
            daniss Daniele Cordella added a comment - I strongly believe this issue is a general theme issue and not restricted to formal_white only. Please consider to apply it at central level.
            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
            nebgor Aparup Banerjee added a comment -

            Hi Daniele ,
            Can you provide a patch for the central solution?

            Show
            nebgor Aparup Banerjee added a comment - Hi Daniele , Can you provide a patch for the central solution?
            Hide
            daniss Daniele Cordella added a comment -

            Sure.
            Add
            table.flexible .r0, table.generaltable .r0

            {background-color: #F0F0F0;}

            table.flexible .r1, table.generaltable .r1

            {background-color: #FAFAFA;}

            to HEAD/theme/canvas/style/tables.css

            Almost all themes inherit from canvas.
            For the other, afaik, an personal solution is needed.

            Show
            daniss Daniele Cordella added a comment - Sure. Add table.flexible .r0, table.generaltable .r0 {background-color: #F0F0F0;} table.flexible .r1, table.generaltable .r1 {background-color: #FAFAFA;} to HEAD/theme/canvas/style/tables.css Almost all themes inherit from canvas. For the other, afaik, an personal solution is needed.
            Hide
            daniss Daniele Cordella added a comment -

            Ciao Aparup. I apologise. Yesterday I was traveling.
            Maybe you were looking for this?
            https://github.com/kordan/moodle/compare/master...MDL-31712_generalsolution_master
            If you are still short of time, I can post the same general/central solution even for M20, M21 and M22.
            Let me know, please. Ciao.

            Show
            daniss Daniele Cordella added a comment - Ciao Aparup. I apologise. Yesterday I was traveling. Maybe you were looking for this? https://github.com/kordan/moodle/compare/master...MDL-31712_generalsolution_master If you are still short of time, I can post the same general/central solution even for M20, M21 and M22. Let me know, please. Ciao.
            Hide
            nebgor Aparup Banerjee added a comment -

            Hi Daniele,
            i had tried the general fix but when testing, it didn't show row colours, i'll try your patch this time, thanks.

            (i can easily cherry-pick those to other branches needing the patch so no worries there)

            Show
            nebgor Aparup Banerjee added a comment - Hi Daniele, i had tried the general fix but when testing, it didn't show row colours, i'll try your patch this time, thanks. (i can easily cherry-pick those to other branches needing the patch so no worries there)
            Hide
            daniss Daniele Cordella added a comment -

            To be honest, I never tested the general patch.
            On the opposite side, I am using the patch for FW from few days and all is working fine since the first time.

            Show
            daniss Daniele Cordella added a comment - To be honest, I never tested the general patch. On the opposite side, I am using the patch for FW from few days and all is working fine since the first time.
            Hide
            daniss Daniele Cordella added a comment -

            Once the color alternation is integrated some core icon will become inadequate because of their white background.
            For instance, the head/pix/t/copy.gif icon uses white background and over a non white row background color it is not nice.
            Attached is the copy.gif icon I use with satisfaction.

            Show
            daniss Daniele Cordella added a comment - Once the color alternation is integrated some core icon will become inadequate because of their white background. For instance, the head/pix/t/copy.gif icon uses white background and over a non white row background color it is not nice. Attached is the copy.gif icon I use with satisfaction.
            Hide
            nebgor Aparup Banerjee added a comment -

            ah thanks for mentioning that. i'll stick with these changes being only for this theme (for the .21+,2.2+ and master).

            I've created MDL-31847.

            This looks good to me (and works), i'll push this up into integration on tuesday for testing.

            ps: 2.0 branch is in security support phase now.

            Show
            nebgor Aparup Banerjee added a comment - ah thanks for mentioning that. i'll stick with these changes being only for this theme (for the .21+,2.2+ and master). I've created MDL-31847 . This looks good to me (and works), i'll push this up into integration on tuesday for testing. ps: 2.0 branch is in security support phase now.
            Hide
            daniss Daniele Cordella added a comment -

            Ciao Aparup.
            I understood the trick.
            The suggestion I posted (to add:
            table.flexible .r0, table.generaltable .r0

            {background-color: #F0F0F0;}

            table.flexible .r1, table.generaltable .r1

            {background-color: #FAFAFA;}

            to HEAD/theme/canvas/style/tables.css)
            is actually working.
            The problem is in FW theme as it can not see these new selectors because of
            theme/formal_white/config.php
            where it is explicitly written:
            $THEME->parents_exclude_sheets = array(
            'canvas'=>array(
            'core',
            'pagelayout',
            'tabs',
            'tables',
            ),
            );
            By changing this to:
            $THEME->parents_exclude_sheets = array(
            'canvas'=>array(
            'core',
            'pagelayout',
            'tabs',
            ),
            );
            all works fine.
            It you like I can post this new solution to my github.
            Let me know. Ciao.

            Show
            daniss Daniele Cordella added a comment - Ciao Aparup. I understood the trick. The suggestion I posted (to add: table.flexible .r0, table.generaltable .r0 {background-color: #F0F0F0;} table.flexible .r1, table.generaltable .r1 {background-color: #FAFAFA;} to HEAD/theme/canvas/style/tables.css) is actually working. The problem is in FW theme as it can not see these new selectors because of theme/formal_white/config.php where it is explicitly written: $THEME->parents_exclude_sheets = array( 'canvas'=>array( 'core', 'pagelayout', 'tabs', 'tables', ), ); By changing this to: $THEME->parents_exclude_sheets = array( 'canvas'=>array( 'core', 'pagelayout', 'tabs', ), ); all works fine. It you like I can post this new solution to my github. Let me know. Ciao.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Daniele,

            I think that, for now, it's ok to apply the changes to FW linked above (add the 2 missing flexible table selectors). Later, when MDL-31847 (the general issue) is fixed, you may reconsider going back in FW and do the $THEME->parents_exclude_sheets alternative, if you want. But for now, I think your original proposal is ok (to be applied to 21, 22 and master).

            Are you ok with that... or do you prefer to wait till MDL-31847 is fixed?

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Daniele, I think that, for now, it's ok to apply the changes to FW linked above (add the 2 missing flexible table selectors). Later, when MDL-31847 (the general issue) is fixed, you may reconsider going back in FW and do the $THEME->parents_exclude_sheets alternative, if you want. But for now, I think your original proposal is ok (to be applied to 21, 22 and master). Are you ok with that... or do you prefer to wait till MDL-31847 is fixed? Ciao
            Hide
            daniss Daniele Cordella added a comment -

            Sure Eloy. In this way moodle get a "more accessible" theme just now and for ever.
            With the time all the other themes will follow and I will change my patch dropping off 'tables' from FW $THEME->parents_exclude_sheets.
            My +1 for this "alternate colors" roadmap.

            Show
            daniss Daniele Cordella added a comment - Sure Eloy. In this way moodle get a "more accessible" theme just now and for ever. With the time all the other themes will follow and I will change my patch dropping off 'tables' from FW $THEME->parents_exclude_sheets. My +1 for this "alternate colors" roadmap.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (getting onto this)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (getting onto this)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks! (21, 22 & master)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 & master)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Passing, while integrating I picked formal_white on all branches and it shows flexitable rows with alternate bg colors.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing, while integrating I picked formal_white on all branches and it shows flexitable rows with alternate bg colors.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

            icao_reverse('arreis olik rebemevon afla letoh ognat');

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao
            Hide
            daniss Daniele Cordella added a comment - - edited

            Eloy....
            can I open an issue for the issue thanks?

             icao_reverse('arreis olik rebemevon afla letoh ognat');

            has a syntax error!
            It should be:

             icao_reverse('arreis olik rebmevon afla letoh ognat');


            tty great man.

            Show
            daniss Daniele Cordella added a comment - - edited Eloy.... can I open an issue for the issue thanks? icao_reverse('arreis olik rebemevon afla letoh ognat'); has a syntax error! It should be: icao_reverse('arreis olik rebmevon afla letoh ognat'); tty great man.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Mar/12