Moodle
  1. Moodle
  2. MDL-31847

row highlighting in tables missing from core base/canvas themes.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      1) Test Standard theme for tables (user accounts page) and see that they are highlighted between rows.
      2) Test that no graphic conflicts occur due to highlighting.

      Show
      1) Test Standard theme for tables (user accounts page) and see that they are highlighted between rows. 2) Test that no graphic conflicts occur due to highlighting.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-31847_master
    • Rank:
      38489

      Description

      while on MDL-31712, Daniele Cordella mentioned that row highlighting should be in core themes. It makes sense too, but i couldn't find a relevant issue so here it is.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side note: Some themes, Anomaly, for example and also Formal White (since MDL-31712) already have own alternation CSS selectors.

          Also, I'm not sure if base or canvas should define them at all (or perhaps yes defaulting to bg-color = none or so), just to allow others to know about their existance.

          Or, alternatively perhaps all core themes should specify them, but always independently. From my knowledge base and canvas use to be color-free (detail-free). And this, although really important because it helps a lot visually, it's just one color detail.

          So, sided-mind here... I leave this on expert hands (anybody but me). Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Side note: Some themes, Anomaly, for example and also Formal White (since MDL-31712 ) already have own alternation CSS selectors. Also, I'm not sure if base or canvas should define them at all (or perhaps yes defaulting to bg-color = none or so), just to allow others to know about their existance. Or, alternatively perhaps all core themes should specify them, but always independently. From my knowledge base and canvas use to be color-free (detail-free). And this, although really important because it helps a lot visually, it's just one color detail. So, sided-mind here... I leave this on expert hands (anybody but me). Ciao
          Hide
          Daniele Cordella added a comment - - edited

          Yes, Eloy.
          I thought about the same problem.
          The alternation of colors is a general issue but the particular color of each theme is a not general at all.
          I see two ways:
          -> the one you proposed: all core themes should specify the row highlight independently.
          -> each theme has to provide two colors in its setting page (or hardcoded in its config) and the general highlight will use them

          IMHO your solution is faster unless deeper afterthought about themes is going to come.

          Show
          Daniele Cordella added a comment - - edited Yes, Eloy. I thought about the same problem. The alternation of colors is a general issue but the particular color of each theme is a not general at all. I see two ways: -> the one you proposed: all core themes should specify the row highlight independently. -> each theme has to provide two colors in its setting page (or hardcoded in its config) and the general highlight will use them IMHO your solution is faster unless deeper afterthought about themes is going to come.
          Hide
          Aparup Banerjee added a comment -

          i think the row colour alternation is a general accessibility issue so this should be addressed in core - as far as readability should be ensured.

          Show
          Aparup Banerjee added a comment - i think the row colour alternation is a general accessibility issue so this should be addressed in core - as far as readability should be ensured.
          Show
          Daniele Cordella added a comment - - edited I wrote a proposal of general solution in: https://github.com/kordan/moodle/tree/MDL-31712_generalsolution_master or here: https://github.com/kordan/moodle/compare/master...MDL-31712_generalsolution_master if you prefer
          Hide
          Daniele Cordella added a comment -

          Aparup and Mary
          I wrote my proposal of general management of each color related problem in http://moodle.org/mod/forum/discuss.php?d=201604

          Show
          Daniele Cordella added a comment - Aparup and Mary I wrote my proposal of general management of each color related problem in http://moodle.org/mod/forum/discuss.php?d=201604
          Hide
          Mary Evans added a comment -

          Thanks Danielle

          Show
          Mary Evans added a comment - Thanks Danielle
          Hide
          Mary Evans added a comment -

          This issue had never been Triaged so this has not been picked up and could do with going into 2.4...

          It's triaged now.

          Show
          Mary Evans added a comment - This issue had never been Triaged so this has not been picked up and could do with going into 2.4... It's triaged now.
          Hide
          Dan Poltawski added a comment -

          I've integrated this now, thanks Mary.

          Show
          Dan Poltawski added a comment - I've integrated this now, thanks Mary.
          Hide
          Mark Nelson added a comment -

          At first I did not notice a difference in colour whatsoever. I then looked at the commit and then at the CSS being applied on the page for rows r0 and r1 and saw they were using #F0F0F0 and #FAFAFA specified in the diff. However, really these colours appear identical. I am failing because I do not believe the difference in colours is enough for the human eye to distinguish between the two, not because the logic failed.

          Show
          Mark Nelson added a comment - At first I did not notice a difference in colour whatsoever. I then looked at the commit and then at the CSS being applied on the page for rows r0 and r1 and saw they were using #F0F0F0 and #FAFAFA specified in the diff. However, really these colours appear identical. I am failing because I do not believe the difference in colours is enough for the human eye to distinguish between the two, not because the logic failed.
          Hide
          Mary Evans added a comment - - edited

          Mark, I think you may need to get yours eyes tested. This is a perfect combination of row background colour.
          See attached image.

          Show
          Mary Evans added a comment - - edited Mark, I think you may need to get yours eyes tested. This is a perfect combination of row background colour. See attached image.
          Hide
          Dan Poltawski added a comment -

          Mary: I looked at this too, but I couldn't see it either!

          Show
          Dan Poltawski added a comment - Mary: I looked at this too, but I couldn't see it either!
          Hide
          Mary Evans added a comment -

          Do either of you see the light-grey background when you hover over the comments here? Because that is the same colour #F0F0F0 which is what I have for row r0. What if I keep that for r0 and make the r1 row white? Would that be better?

          Show
          Mary Evans added a comment - Do either of you see the light-grey background when you hover over the comments here? Because that is the same colour #F0F0F0 which is what I have for row r0. What if I keep that for r0 and make the r1 row white? Would that be better?
          Hide
          Daniele Cordella added a comment - - edited

          There are two versions of the Web Content Accessibility Guidelines to verify if two colors are well contrasted or not.
          I googled "html accessibility color contrast analyser" and, for instance, I found: http://www.colorsontheweb.com/colorcontrast.asp
          That site declares that #F0F0F0 and #FAFAFA are not ok.

          Show
          Daniele Cordella added a comment - - edited There are two versions of the Web Content Accessibility Guidelines to verify if two colors are well contrasted or not. I googled "html accessibility color contrast analyser" and, for instance, I found: http://www.colorsontheweb.com/colorcontrast.asp That site declares that #F0F0F0 and #FAFAFA are not ok.
          Hide
          Mary Evans added a comment -

          Un'alternativa sarebbe bene allora

          Show
          Mary Evans added a comment - Un'alternativa sarebbe bene allora
          Hide
          Mary Evans added a comment -

          @Dan

          I've altered this so only r0 uses the background-color, this measn r1 will retain the default color (white).

          Show
          Mary Evans added a comment - @Dan I've altered this so only r0 uses the background-color, this measn r1 will retain the default color (white).
          Hide
          Dan Poltawski added a comment -

          Sam pointed out that this rule is defined in standard theme:
          .generaltable .cell

          {background-color: #FFFFFF;}
          Show
          Dan Poltawski added a comment - Sam pointed out that this rule is defined in standard theme: .generaltable .cell {background-color: #FFFFFF;}
          Hide
          Dan Poltawski added a comment -

          I'm passing this based on other themes, I think we need to deal with the issues from this change in a new issue.

          Show
          Dan Poltawski added a comment - I'm passing this based on other themes, I think we need to deal with the issues from this change in a new issue.
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: