Moodle
  1. Moodle
  2. MDL-31712

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      38299

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - Hi Daniele , Can you provide a patch for the central solution?
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          (getting onto this)

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

          Integrated, thanks! (21, 22 & master)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 & master)
          Hide
          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
          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
          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
          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
          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
          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: