Moodle
  1. Moodle
  2. MDL-37771

Edit Quiz: background color sometimes hides the text as the text color is similar

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1
    • Fix Version/s: 2.3.6, 2.4.3
    • Component/s: Quiz, Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Clear all caches
      2. Go to the 'Edit quiz' page for any quiz.
      3. Make sure that the question bank contents are visible. (If not, click [Show].)
      4. Make sure there are several questions shown in the question bank. (If not, add some.)
      5. Now look at the page in every theme, and make sure the styling of the question bank display is OK.
      Show
      Clear all caches Go to the 'Edit quiz' page for any quiz. Make sure that the question bank contents are visible. (If not, click [Show] .) Make sure there are several questions shown in the question bank. (If not, add some.) Now look at the page in every theme, and make sure the styling of the question bank display is OK.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-37771-M24
    • Pull Master Branch:
    • Rank:
      47501

      Description

      This bug was found during testing MDL-29723

      See screen capture. Need to check all themes.

      1. MDL-37771.jpg
        78 kB
      2. quiz.png
        371 kB
      3. Screenshot_30_01_13_12_03_PM.png
        179 kB
      4. Screenshot_30_01_13_12_20_PM.png
        162 kB

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Mary, do you want to look at this, or shall I? Basically, if you want, feel free to assign this to yourself.

          Show
          Tim Hunt added a comment - Mary, do you want to look at this, or shall I? Basically, if you want, feel free to assign this to yourself.
          Hide
          Tim Hunt added a comment -

          OK, Mary, this seems to fix it for me.

          I explained the changes I made in the commit comment. https://github.com/timhunt/moodle/commit/9d9de8437cd55fcdc3e1d07df986ff66abfa9af8

          Please could you take a look and let me know what you think.

          Show
          Tim Hunt added a comment - OK, Mary, this seems to fix it for me. I explained the changes I made in the commit comment. https://github.com/timhunt/moodle/commit/9d9de8437cd55fcdc3e1d07df986ff66abfa9af8 Please could you take a look and let me know what you think.
          Hide
          Dan Poltawski added a comment -

          Hi Tim,

          I know you were after Mary reviewing this but she hasn't seem to be able to yet so I took a look.

          I think your solution makes sense. It would be great to have Mary confirm her support for it too. So if required please assign her as peer reviewer.

          Show
          Dan Poltawski added a comment - Hi Tim, I know you were after Mary reviewing this but she hasn't seem to be able to yet so I took a look. I think your solution makes sense. It would be great to have Mary confirm her support for it too. So if required please assign her as peer reviewer.
          Hide
          Tim Hunt added a comment -

          Looking again at the fix, it still makes sense to me, so submitting for integration.

          I still would like it if Mary could give it the OK too, but she has a lot of other bugs, so I won't wait.

          Show
          Tim Hunt added a comment - Looking again at the fix, it still makes sense to me, so submitting for integration. I still would like it if Mary could give it the OK too, but she has a lot of other bugs, so I won't wait.
          Hide
          Mary Evans added a comment -

          I don't know how I missed this!
          You should have sent me a message.

          I'll take a look now.

          Show
          Mary Evans added a comment - I don't know how I missed this! You should have sent me a message. I'll take a look now.
          Hide
          Mary Evans added a comment - - edited

          This should have been assigned to me as it's a Theme BUG. However, the problem stems from the class header.

          This would be my way of dealing with this:

          /* Question Bank
          -------------------------*/
          
          #page-mod-quiz-edit .questionbankwindow.block div.header,
          #page-mod-quiz-edit table#categoryquestions thead .header{
              background-color: #69804e;
          }
          
          #page-mod-quiz-edit table#categoryquestions thead .header,
          #page-mod-quiz-edit table#categoryquestions thead .header a:link,
          #page-mod-quiz-edit table#categoryquestions thead .header a:visited {
              color: #fff;
          }
          
          #page-mod-quiz-edit table#categoryquestions thead .header a:hover {
              text-decoration: underline;
          }
          
          Show
          Mary Evans added a comment - - edited This should have been assigned to me as it's a Theme BUG. However, the problem stems from the class header. This would be my way of dealing with this: /* Question Bank -------------------------*/ #page-mod-quiz-edit .questionbankwindow.block div.header, #page-mod-quiz-edit table#categoryquestions thead .header{ background-color: #69804e; } #page-mod-quiz-edit table#categoryquestions thead .header, #page-mod-quiz-edit table#categoryquestions thead .header a:link, #page-mod-quiz-edit table#categoryquestions thead .header a:visited { color: #fff; } #page-mod-quiz-edit table#categoryquestions thead .header a:hover { text-decoration: underline; }
          Hide
          Tim Hunt added a comment -

          Your proposal would fix this specific instance of the problem, but would not fix other problems caused by the over-broad .header rules that are ambiguous.

          Up to you, do you wan to make a fix your way, or shall I submit mine for integration?

          Show
          Tim Hunt added a comment - Your proposal would fix this specific instance of the problem, but would not fix other problems caused by the over-broad .header rules that are ambiguous. Up to you, do you wan to make a fix your way, or shall I submit mine for integration?
          Hide
          Mary Evans added a comment - - edited

          In actual fact you could add the above fix changeing the background-color to dark blue to match the Question Bank header, and make the hover white too. Like so...

          /* Question Bank
          -------------------------*/
          
          #page-mod-quiz-edit .questionbankwindow.block div.header,
          #page-mod-quiz-edit table#categoryquestions thead .header{
              background-color: #009;
          }
          
          #page-mod-quiz-edit table#categoryquestions thead .header,
          #page-mod-quiz-edit table#categoryquestions thead .header a:link,
          #page-mod-quiz-edit table#categoryquestions thead .header a:visited,
          #page-mod-quiz-edit table#categoryquestions thead .header a:hover {
              color: #fff;
          }
          #page-mod-quiz-edit table#categoryquestions thead .header a:hover {
              text-decoration: underline;
          }
          

          Which would (or should) fix all themes.

          Show
          Mary Evans added a comment - - edited In actual fact you could add the above fix changeing the background-color to dark blue to match the Question Bank header, and make the hover white too. Like so... /* Question Bank -------------------------*/ #page-mod-quiz-edit .questionbankwindow.block div.header, #page-mod-quiz-edit table#categoryquestions thead .header{ background-color: #009; } #page-mod-quiz-edit table#categoryquestions thead .header, #page-mod-quiz-edit table#categoryquestions thead .header a:link, #page-mod-quiz-edit table#categoryquestions thead .header a:visited, #page-mod-quiz-edit table#categoryquestions thead .header a:hover { color: #fff; } #page-mod-quiz-edit table#categoryquestions thead .header a:hover { text-decoration: underline; } Which would (or should) fix all themes.
          Hide
          Tim Hunt added a comment -

          I think the dark blue is a weird anomaly. If anything we should be getting rid of that, not adding more of it.

          Show
          Tim Hunt added a comment - I think the dark blue is a weird anomaly. If anything we should be getting rid of that, not adding more of it.
          Hide
          Mary Evans added a comment -

          It looks OK in the attached file as it would look in Boxxie, if the CSS went into Base theme.

          Show
          Mary Evans added a comment - It looks OK in the attached file as it would look in Boxxie, if the CSS went into Base theme.
          Hide
          Tim Hunt added a comment -

          You are right. That looks better. Feel free to submit that for integration. Thanks.

          Show
          Tim Hunt added a comment - You are right. That looks better. Feel free to submit that for integration. Thanks.
          Hide
          Mary Evans added a comment -

          Will do.
          I presume you mean for me to take over here?
          I've just assigned it to myself and will do the GIT stuff later.

          Show
          Mary Evans added a comment - Will do. I presume you mean for me to take over here? I've just assigned it to myself and will do the GIT stuff later.
          Hide
          Mary Evans added a comment - - edited

          By the way...I noticed that the tiny arrow that indicates the sort direction, tends to switch sides every so often. Have you noticed that? You can see in the image I uploaded that it's on the right, normally its on the left.

          Show
          Mary Evans added a comment - - edited By the way...I noticed that the tiny arrow that indicates the sort direction, tends to switch sides every so often. Have you noticed that? You can see in the image I uploaded that it's on the right, normally its on the left.
          Hide
          Tim Hunt added a comment -

          Great! Thank you very much Mary. I will try to do you a quick peer-review after you have done your git thing.

          Presumably this should not be 'waiting for integration review' yet?

          Show
          Tim Hunt added a comment - Great! Thank you very much Mary. I will try to do you a quick peer-review after you have done your git thing. Presumably this should not be 'waiting for integration review' yet?
          Hide
          Mary Evans added a comment -

          Reopening to start development

          Show
          Mary Evans added a comment - Reopening to start development
          Hide
          Mary Evans added a comment -

          Tim can you Peer Review this in Master? Cheers

          Show
          Mary Evans added a comment - Tim can you Peer Review this in Master? Cheers
          Hide
          Tim Hunt added a comment -

          The only problem is the comment, These are far from the only rules affecting the question bank, aren't they?

          Show
          Tim Hunt added a comment - The only problem is the comment, These are far from the only rules affecting the question bank, aren't they?
          Hide
          Mary Evans added a comment -

          The commit comment or the one in the stylesheet.

          Show
          Mary Evans added a comment - The commit comment or the one in the stylesheet.
          Hide
          Tim Hunt added a comment -

          Sorry not to be clear, I am looking at what I see when I click the three "Pull ... Diff URL" links above.

          1. I just noticed that the master branch is different from the other two. The

          #page-mod-quiz-edit table#categoryquestions thead .header a:hover {

          line is missing.

          2. My concern is that the new rules you added are specific to the quiz, shouldn't they be in mod/quiz/styles.php instead? I think there are already some other similar rules around https://github.com/lazydaisy/moodle/blob/5803020eba191e37046a2bf8fd5891d4c742a1d2/mod/quiz/styles.css#L283.

          3. The other problem with the comment is that I have not seen that style of commenting used anywhere else in Moodle. For example, look at the other comments in theme/base/style/question.css or mod/quiz/styles.css

          Show
          Tim Hunt added a comment - Sorry not to be clear, I am looking at what I see when I click the three "Pull ... Diff URL" links above. 1. I just noticed that the master branch is different from the other two. The #page-mod-quiz-edit table#categoryquestions thead .header a:hover { line is missing. 2. My concern is that the new rules you added are specific to the quiz, shouldn't they be in mod/quiz/styles.php instead? I think there are already some other similar rules around https://github.com/lazydaisy/moodle/blob/5803020eba191e37046a2bf8fd5891d4c742a1d2/mod/quiz/styles.css#L283 . 3. The other problem with the comment is that I have not seen that style of commenting used anywhere else in Moodle. For example, look at the other comments in theme/base/style/question.css or mod/quiz/styles.css
          Hide
          Mary Evans added a comment -

          Ah, I see we have been talking at cross purposes.

          I had assumed you wanted me to add this to base as I had suggested so that the style of the Q.B.W whould be the same across the board, as far as themes were concerned. That way it's easier to override.

          However to keep thing simple I'll add them to the mod/quiz/styles.css - no problem.

          As for the comment in the css. That is a style I have used for a long time and you are the first to comment, so this shows you are perhaps more observant than most!

          I like that style as it is easier for me to see the different headings
          For example:

          /**generalbox**/
          .generalbox {border: 1px solid #ddd}

          is not as easy to see in a sea of css
          as the comment style which I prefer below

          /* generalbox
          -----------------*/
          .generalbox {border: 1px solid #ddd}
          Show
          Mary Evans added a comment - Ah, I see we have been talking at cross purposes. I had assumed you wanted me to add this to base as I had suggested so that the style of the Q.B.W whould be the same across the board, as far as themes were concerned. That way it's easier to override. However to keep thing simple I'll add them to the mod/quiz/styles.css - no problem. As for the comment in the css. That is a style I have used for a long time and you are the first to comment, so this shows you are perhaps more observant than most! I like that style as it is easier for me to see the different headings For example: /**generalbox**/ .generalbox {border: 1px solid #ddd} is not as easy to see in a sea of css as the comment style which I prefer below /* generalbox -----------------*/ .generalbox {border: 1px solid #ddd}
          Hide
          Tim Hunt added a comment -

          Comment style is really up to the integrators. If they are happy with your style, that is OK.

          The strange dark blue colour is only used on the quiz edit page, so I think it is OK to just apply these styles in the quiz.

          Is guess this is about ready for integration.

          Show
          Tim Hunt added a comment - Comment style is really up to the integrators. If they are happy with your style, that is OK. The strange dark blue colour is only used on the quiz edit page, so I think it is OK to just apply these styles in the quiz. Is guess this is about ready for integration.
          Hide
          Tim Hunt added a comment -

          Sorry, you are probably about ready to kill me at this point, but I have just remembered MDL-38209. If you submit this for integration this week, we will get horrible merge conflicts. See https://github.com/timhunt/moodle/compare/master...MDL-38209 - I basically completely re-wrote mod/quiz/styles.css to fix the coding style.

          Show
          Tim Hunt added a comment - Sorry, you are probably about ready to kill me at this point, but I have just remembered MDL-38209 . If you submit this for integration this week, we will get horrible merge conflicts. See https://github.com/timhunt/moodle/compare/master...MDL-38209 - I basically completely re-wrote mod/quiz/styles.css to fix the coding style.
          Hide
          Mary Evans added a comment -

          Don't worry about it. I'm in a similar conflict with some MyMobile theme changes. This just adds to the fun of it all.

          Good job you said, I have only done the master branch so I can delete that and wait until MDL-38209 is integrated.

          Have you re-based MDL-38209 yet? I don't think you will have any problems with it.

          Show
          Mary Evans added a comment - Don't worry about it. I'm in a similar conflict with some MyMobile theme changes. This just adds to the fun of it all. Good job you said, I have only done the master branch so I can delete that and wait until MDL-38209 is integrated. Have you re-based MDL-38209 yet? I don't think you will have any problems with it.
          Hide
          Tim Hunt added a comment -

          Yes, I rebased it, and did have a problem with a commit of Fred's from last week.

          Show
          Tim Hunt added a comment - Yes, I rebased it, and did have a problem with a commit of Fred's from last week.
          Hide
          Mary Evans added a comment -

          MDL-38209 Passed Testing!

          So I can fix this now when Moodle has been updated...yes?

          Show
          Mary Evans added a comment - MDL-38209 Passed Testing! So I can fix this now when Moodle has been updated...yes?
          Hide
          Mary Evans added a comment -

          Tim,

          I have just been testing this as I am working on it and it's not working. In fact it is creating more problems in themes!

          It would be better just to adjust each theme, as I would end up having to do that anyway if I changed this in quiz/style.css.

          Show
          Mary Evans added a comment - Tim, I have just been testing this as I am working on it and it's not working. In fact it is creating more problems in themes! It would be better just to adjust each theme, as I would end up having to do that anyway if I changed this in quiz/style.css.
          Hide
          Tim Hunt added a comment -

          I am happy with this if you are.

          Show
          Tim Hunt added a comment - I am happy with this if you are.
          Hide
          Mary Evans added a comment - - edited

          As you will have seen I've done the fixes and submitted them for integration.
          I've just added one tiny change as a new commit to fix a typo. Do you think that will be OK or will I need to squash/merge them?

          Show
          Mary Evans added a comment - - edited As you will have seen I've done the fixes and submitted them for integration. I've just added one tiny change as a new commit to fix a typo. Do you think that will be OK or will I need to squash/merge them?
          Hide
          Tim Hunt added a comment -

          I think you should squash the commits. Thanks.

          Show
          Tim Hunt added a comment - I think you should squash the commits. Thanks.
          Hide
          Mary Evans added a comment -

          Done!

          Of all the themes in these fixes Serenity looks the best.

          Show
          Mary Evans added a comment - Done! Of all the themes in these fixes Serenity looks the best.
          Hide
          Damyon Wiese added a comment -

          Thanks Mary and Tim,

          This is integrated now on master, 24 and 23. Just a note: I had to amend the git commit message on master as it was missing the blank line after the first line. This is important because lots of git commands are expecting it (like rebase).

          I added a step to the testing instructions to purge caches as I didn't add a version bump for this.

          Show
          Damyon Wiese added a comment - Thanks Mary and Tim, This is integrated now on master, 24 and 23. Just a note: I had to amend the git commit message on master as it was missing the blank line after the first line. This is important because lots of git commands are expecting it (like rebase). I added a step to the testing instructions to purge caches as I didn't add a version bump for this.
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim and Mary,

          Most themes doesn't have background color in question, except magazine. But text looks fine (white over green)

          FYI: It seems brick theme is not rendering question bank properly (See attached image), and it's same in stable.

          Show
          Rajesh Taneja added a comment - Thanks Tim and Mary, Most themes doesn't have background color in question, except magazine. But text looks fine (white over green) FYI: It seems brick theme is not rendering question bank properly (See attached image), and it's same in stable.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: