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

      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.

        Gliffy Diagrams

          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: