Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Questions, Themes
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      • Set up a quiz with a few questions
      • Attempt the quiz with a few students

      Test steps

      1. Explore the quiz reports and review pages
      2. Explore the question bank
      3. Make sure the icons are consistent and the PNG fallback works

      Some more test

      Hate me for this

      1. Look at the diff and make your way through to the new icons
      2. The important thing here is to make sure nothing broke and I didn't get terribly wrong while altering the icons
      3. Do not spend days trying to get there
      Show
      Test pre-requisites Set up a quiz with a few questions Attempt the quiz with a few students Test steps Explore the quiz reports and review pages Explore the question bank Make sure the icons are consistent and the PNG fallback works Some more test Hate me for this Look at the diff and make your way through to the new icons The important thing here is to make sure nothing broke and I didn't get terribly wrong while altering the icons Do not spend days trying to get there
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36638-master

      Description

      Quiz administration block has coloured preview icon.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            The preview icon is a fairly standard icon used in numerous places throughout Moodle. Fixing that is a core icons thing so reassigning to Barbara.

            The ticks and crosses should be green / yellow / red. That is a feature.

            Show
            timhunt Tim Hunt added a comment - The preview icon is a fairly standard icon used in numerous places throughout Moodle. Fixing that is a core icons thing so reassigning to Barbara. The ticks and crosses should be green / yellow / red. That is a feature.
            Hide
            timhunt Tim Hunt added a comment -

            To be precise, that icon is 't/preview'

            Show
            timhunt Tim Hunt added a comment - To be precise, that icon is 't/preview'
            Hide
            fred Frédéric Massart added a comment -

            We need a i/preview to use t/preview in size 16x16.

            Show
            fred Frédéric Massart added a comment - We need a i/preview to use t/preview in size 16x16.
            Hide
            fred Frédéric Massart added a comment -

            Note: the code I removed from the quiz is based on a deprecated argument.

            Show
            fred Frédéric Massart added a comment - Note: the code I removed from the quiz is based on a deprecated argument.
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, but this sucks. -1

            1. why did you spend time renaming these icons? That seems pointless to me, and risks introducing regressions. I am not saying that the current names are great. Just that they are stable and work.
            2. the difference between flagged and unflagged is not enough. We experimented with icons like this during development, and quickly realised that from a usability point of view, flagged really needs to maintain the red colour. Not that this ties in with how the flag appears in the navigation block.
            3. How many contrib question types did you check, to see if they were still using that deprecated argument? That really comes back to my point that renaming the icons was a waste of time.
            4. If you must rename the icons, the words that are used everywhere else in the code are correct, partiallycorrect and incorrect. Making two new names for each icon also strikes me as silly.
            Show
            timhunt Tim Hunt added a comment - Sorry, but this sucks. -1 why did you spend time renaming these icons? That seems pointless to me, and risks introducing regressions. I am not saying that the current names are great. Just that they are stable and work. the difference between flagged and unflagged is not enough. We experimented with icons like this during development, and quickly realised that from a usability point of view, flagged really needs to maintain the red colour. Not that this ties in with how the flag appears in the navigation block. How many contrib question types did you check, to see if they were still using that deprecated argument? That really comes back to my point that renaming the icons was a waste of time. If you must rename the icons, the words that are used everywhere else in the code are correct, partiallycorrect and incorrect. Making two new names for each icon also strikes me as silly.
            Hide
            fred Frédéric Massart added a comment -

            Hi Tim,

            I understand that you might not fully agree with the changes we proposed here, but saying that it sucks is maybe a bit harsh. We have made a lot of changes all over the place, and we are definitely happy to find the best solution. I understand that we might have failed from your point of view, I just don't think we sucked .

            1/ Sure, there are always risks, but there should not be regression. For now we have marked some icons as deprecated but deleting them as not been considered at all in the 2.4 release. If we want to go towards some improvement regarding theming, we have to opt for better names which target what the icons are used for more than what they represent. If themers want to change the red cross to a red spot, they will affect other areas than quiz/questions which they might not be aware of and might not make sense at all. The perfect example being the delete button named 'cross' which is used for both 'close' and 'delete' actions.

            2/ This is a tricky problem. The new icons guide lines specified a monochrome use only. That's what we tried to achieve until we reached the tick_green, tick_amber and cross_red which obviously needed some colour to trigger the attention more than a monochrome icon would do. We obviously can make the flag in colour, but would this breach to the guide lines be really what we want to do? I am nobody to answer that question, I just think that this is an important decision to make now, not to have any regrets in the future.

            3/ As said before, the deprecated icons are still in the core and none of them would break, they would just use the previous icon instead of the new one, although we could create a temporary tick_green, tick_amber and cross_red which would be a copy of the new icons mark_*. We could even make that generic for all deprecated icons so any fallback would be done nicely, and then the actual remove of the icon could happen in later versions. I didn't think of that eventuality/possibility before, thanks for raising that point.

            4/ That's a good point, I have to admit that I didn't look close enough as I was trying to come up with new names, what would you think of mark_correct, mark_partial and mark_incorrect? I like having a strict distinction of the icon use in its name, therefore we would avoid people using correct because it's a tick green that could evolve in something completely different somewhere else.

            Thanks Tim, hope you will excuse the too little time we allowed to that issue in the first place, there are still quite some places to work on and we're getting short on time (understand stressed and overwhelmed ).

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Tim, I understand that you might not fully agree with the changes we proposed here, but saying that it sucks is maybe a bit harsh. We have made a lot of changes all over the place, and we are definitely happy to find the best solution. I understand that we might have failed from your point of view, I just don't think we sucked . 1/ Sure, there are always risks, but there should not be regression. For now we have marked some icons as deprecated but deleting them as not been considered at all in the 2.4 release. If we want to go towards some improvement regarding theming, we have to opt for better names which target what the icons are used for more than what they represent. If themers want to change the red cross to a red spot, they will affect other areas than quiz/questions which they might not be aware of and might not make sense at all. The perfect example being the delete button named 'cross' which is used for both 'close' and 'delete' actions. 2/ This is a tricky problem. The new icons guide lines specified a monochrome use only. That's what we tried to achieve until we reached the tick_green, tick_amber and cross_red which obviously needed some colour to trigger the attention more than a monochrome icon would do. We obviously can make the flag in colour, but would this breach to the guide lines be really what we want to do? I am nobody to answer that question, I just think that this is an important decision to make now, not to have any regrets in the future. 3/ As said before, the deprecated icons are still in the core and none of them would break, they would just use the previous icon instead of the new one, although we could create a temporary tick_green, tick_amber and cross_red which would be a copy of the new icons mark_*. We could even make that generic for all deprecated icons so any fallback would be done nicely, and then the actual remove of the icon could happen in later versions. I didn't think of that eventuality/possibility before, thanks for raising that point. 4/ That's a good point, I have to admit that I didn't look close enough as I was trying to come up with new names, what would you think of mark_correct , mark_partial and mark_incorrect ? I like having a strict distinction of the icon use in its name, therefore we would avoid people using correct because it's a tick green that could evolve in something completely different somewhere else. Thanks Tim, hope you will excuse the too little time we allowed to that issue in the first place, there are still quite some places to work on and we're getting short on time (understand stressed and overwhelmed ). Cheers, Fred
            Hide
            fred Frédéric Massart added a comment -

            (Also, in 2.5 we could create a check on debug level and themers options to return an noticeable image like a red box for deprecated icons, just to trigger themers/developers attention on those.)

            Show
            fred Frédéric Massart added a comment - (Also, in 2.5 we could create a check on debug level and themers options to return an noticeable image like a red box for deprecated icons, just to trigger themers/developers attention on those.)
            Hide
            timhunt Tim Hunt added a comment -

            Apologies for any offence caused but the word sucks.

            1/3/4. I don't like the word mark. That has a specific meaning in the question code: http://docs.moodle.org/dev/Overview_of_the_Moodle_question_engine#A_note_about_scores

            If you really want to change the names, then I think question_correct, question_partiallycorrect and question_incorrect is the best option. Or, possibly, qstate_correct etc. Yes, the partiallycorrect name gets rather verbose, but I think consistency with other parts of the code is important.

            With that consistency, you can replace the if statement in question/type/rendererbase.php with

            $icon = 'question_' . $state->get_feedback_class();

            2. You oversimplify when you say "The new icons guide lines specified a monochrome use only." Look at https://moodle.org/course/view.php?id=5. Lots of coloured icons for each activity. Anyway, the point is functionality and usability, not some policy.

            The point of the question flagging feature is to let the student highlight certain questions so that they stand out. A grey icon, that is only a few pixels different from another grey icon, fails to do that.

            Show
            timhunt Tim Hunt added a comment - Apologies for any offence caused but the word sucks. 1/3/4. I don't like the word mark. That has a specific meaning in the question code: http://docs.moodle.org/dev/Overview_of_the_Moodle_question_engine#A_note_about_scores If you really want to change the names, then I think question_correct, question_partiallycorrect and question_incorrect is the best option. Or, possibly, qstate_correct etc. Yes, the partiallycorrect name gets rather verbose, but I think consistency with other parts of the code is important. With that consistency, you can replace the if statement in question/type/rendererbase.php with $icon = 'question_' . $state->get_feedback_class(); 2. You oversimplify when you say "The new icons guide lines specified a monochrome use only." Look at https://moodle.org/course/view.php?id=5 . Lots of coloured icons for each activity. Anyway, the point is functionality and usability, not some policy. The point of the question flagging feature is to let the student highlight certain questions so that they stand out . A grey icon, that is only a few pixels different from another grey icon, fails to do that.
            Hide
            fred Frédéric Massart added a comment -

            (Added Martin as a watcher here)

            1/3/4: From my humble point of view, I think question_* would be too specific and discourage other developers from using those icons for grade results. I don't know what the best name here would be, just raising a point.

            2: I don't think I oversimplify things, only the activity icons were meant to have colours, any other icon was meant to be monochrome. My opinion would be to keep that icon monochrome here as well, but I don't consider my +1 as worth anything here.

            Show
            fred Frédéric Massart added a comment - (Added Martin as a watcher here) 1/3/4: From my humble point of view, I think question_* would be too specific and discourage other developers from using those icons for grade results. I don't know what the best name here would be, just raising a point. 2: I don't think I oversimplify things, only the activity icons were meant to have colours, any other icon was meant to be monochrome. My opinion would be to keep that icon monochrome here as well, but I don't consider my +1 as worth anything here.
            Hide
            nebgor Aparup Banerjee added a comment -

            Hm, at reporting time i had assumed that the monochrome-ness was also colour-blindness accessibility but now i'm not so sure when i think about traffic lights.

            Show
            nebgor Aparup Banerjee added a comment - Hm, at reporting time i had assumed that the monochrome-ness was also colour-blindness accessibility but now i'm not so sure when i think about traffic lights.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Apu: Generally yes, one shouldn't RELY on colours, but there's no reason not to have additional colour if it improves visibility for people who can see it (ie nearly everyone). Colours are very important.

            TICKS:

            Fred, I like the idea of fixing the names and if we keep the old icons around for backward compatibility then I don't see a problem.

            I also agree it makes sense to have generic icons we can use everywhere else in Moodle with:

            • one set of colour icons for valid/invalid
            • one set of colour icons for grade_correct/grade_partial/grade_incorrect to be used generically in Moodle

            (even if they are identical ticks/crosses by default, designers may want to replace the first with happy/sad smileys, for example).

            FLAGS:
            I think the whole flag interface needs some thought at this point, as it's totally not obvious to a novice user what flagging is just by looking at it. However, to be conservative for 2.4 I'd leave it as is until we can give an overhaul of the whole quiz interface (particularly the building of quizzes which I tried to do recently for a real project and was again shocked by the slowness and complexity of it).

            PREVIEW:
            Definitely just needs to be a monochrome core icon that matches the rest of the core icons.

            Show
            dougiamas Martin Dougiamas added a comment - Apu: Generally yes, one shouldn't RELY on colours, but there's no reason not to have additional colour if it improves visibility for people who can see it (ie nearly everyone). Colours are very important. TICKS: Fred, I like the idea of fixing the names and if we keep the old icons around for backward compatibility then I don't see a problem. I also agree it makes sense to have generic icons we can use everywhere else in Moodle with: one set of colour icons for valid/invalid one set of colour icons for grade_correct/grade_partial/grade_incorrect to be used generically in Moodle (even if they are identical ticks/crosses by default, designers may want to replace the first with happy/sad smileys, for example). FLAGS: I think the whole flag interface needs some thought at this point, as it's totally not obvious to a novice user what flagging is just by looking at it. However, to be conservative for 2.4 I'd leave it as is until we can give an overhaul of the whole quiz interface (particularly the building of quizzes which I tried to do recently for a real project and was again shocked by the slowness and complexity of it). PREVIEW: Definitely just needs to be a monochrome core icon that matches the rest of the core icons.
            Hide
            barbararamiro Barbara Ramiro added a comment -

            Martin, just want to clarify about the flag, when you say leave the interface as is, does it mean the layout but not necessarily keeping the old icon shape and color? In short, do we use the proposed monochrome icon?

            Show
            barbararamiro Barbara Ramiro added a comment - Martin, just want to clarify about the flag, when you say leave the interface as is, does it mean the layout but not necessarily keeping the old icon shape and color? In short, do we use the proposed monochrome icon?
            Hide
            timhunt Tim Hunt added a comment -

            1. Fred, if question_correct etc. are too specific, can't we just call these icons correct / partiallycorrect / incorrect

            2. All, please can we stop talking about your monochrome icon policy, and start talking about the purpose of this functionality, what its point is from the users perspective, and hence what will give the best usability.

            It seems obvious to me that a monochrome icon when the flag is off, switching to a red flag - using the same red colour that is used in the quiz navigation block - when the question has been flagged, does provide the desired functionality.

            As I said above, we tried all monochrome icons at one point during the evolution of the feature at the OU. The proposed icons were almost the same as yours. Filled and outline grey flags. It did not work. Please learn from our experience.

            Show
            timhunt Tim Hunt added a comment - 1. Fred, if question_correct etc. are too specific, can't we just call these icons correct / partiallycorrect / incorrect 2. All, please can we stop talking about your monochrome icon policy, and start talking about the purpose of this functionality, what its point is from the users perspective, and hence what will give the best usability. It seems obvious to me that a monochrome icon when the flag is off, switching to a red flag - using the same red colour that is used in the quiz navigation block - when the question has been flagged, does provide the desired functionality. As I said above, we tried all monochrome icons at one point during the evolution of the feature at the OU. The proposed icons were almost the same as yours. Filled and outline grey flags. It did not work. Please learn from our experience.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Barbara I meant don't touch the flag icons at all.

            Show
            dougiamas Martin Dougiamas added a comment - Barbara I meant don't touch the flag icons at all.
            Hide
            fred Frédéric Massart added a comment -

            Hi Tim, pushing for your review again. I followed Martin's proposition of grade_* which I think is both reusable and clearly defined. I reverted the changed to the flag as per his comment to. Thanks!

            Show
            fred Frédéric Massart added a comment - Hi Tim, pushing for your review again. I followed Martin's proposition of grade_* which I think is both reusable and clearly defined. I reverted the changed to the flag as per his comment to. Thanks!
            Hide
            timhunt Tim Hunt added a comment -

            1. Please use
            $icon = 'grade_' . $state->get_feedback_class();
            and don't make up a new inconsistent naming convention.

            2. Well, I would change the flag icons, since what Barbara has drawn has a shape, and uses a grey, that is consistent with everything else. I would just change the colour of the flag (but not the stick) in the flagged image to the same red that was used in the past. But, I don't know how much work that is.

            Hmm. On the other hand, this icon appears on a grey background, so perhaps the standard grey is too pale, and you should indeed leave what we have now.

            5. What is class icon-right?

            6. Are you sure that
            #page-mod-quiz-summary .questionflag

            { vertical-align: text-bottom; }

            looks right. With the old icons, vertical-align: middle; worked better.

            Show
            timhunt Tim Hunt added a comment - 1. Please use $icon = 'grade_' . $state->get_feedback_class(); and don't make up a new inconsistent naming convention. 2. Well, I would change the flag icons, since what Barbara has drawn has a shape, and uses a grey, that is consistent with everything else. I would just change the colour of the flag (but not the stick) in the flagged image to the same red that was used in the past. But, I don't know how much work that is. Hmm. On the other hand, this icon appears on a grey background, so perhaps the standard grey is too pale, and you should indeed leave what we have now. 5. What is class icon-right? 6. Are you sure that #page-mod-quiz-summary .questionflag { vertical-align: text-bottom; } looks right. With the old icons, vertical-align: middle; worked better.
            Hide
            dougiamas Martin Dougiamas added a comment -

            1. $state->get_feedback_class() is quiz-specific code, no? I don't see that core needs to align with quiz names. That said I have no problem with "partiallycorrect" instead of "partial".

            2. Given short timeframe let's just leave it as 2.3.

            Show
            dougiamas Martin Dougiamas added a comment - 1. $state->get_feedback_class() is quiz-specific code, no? I don't see that core needs to align with quiz names. That said I have no problem with "partiallycorrect" instead of "partial". 2. Given short timeframe let's just leave it as 2.3.
            Hide
            fred Frédéric Massart added a comment -

            Thanks everyone, I am sending this for integration. A few notes though:

            • I didn't feel like trying to use get_feedback_class, IMO that method should have nothing to do with icons as it returns a class. Also, I would have had to handle cases where the method could return empty or an non-existing icon. The best solution might be to create a new method in question_state to return the icon to use, but I am not sure that this would be the right place. Anyway, due to the lack of time, I think it's fair to go this way and eventually improve that in the future.
            • The class icon-right has been added in a recent issue to create a step towards some consistency in the whole icon system. They're meant to be used to correctly align and position icons which will be displayed on the left or right of a text.
            • I have used text-bottom everywhere on Barbara's request, to me it looks good.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks everyone, I am sending this for integration. A few notes though: I didn't feel like trying to use get_feedback_class , IMO that method should have nothing to do with icons as it returns a class. Also, I would have had to handle cases where the method could return empty or an non-existing icon. The best solution might be to create a new method in question_state to return the icon to use, but I am not sure that this would be the right place. Anyway, due to the lack of time, I think it's fair to go this way and eventually improve that in the future. The class icon-right has been added in a recent issue to create a step towards some consistency in the whole icon system. They're meant to be used to correctly align and position icons which will be displayed on the left or right of a text. I have used text-bottom everywhere on Barbara's request, to me it looks good. Cheers, Fred
            Hide
            fred Frédéric Massart added a comment - - edited

            (Changed icon-right and icon-left to icon-post and icon-pre in MDL-36837)

            Show
            fred Frédéric Massart added a comment - - edited (Changed icon-right and icon-left to icon-post and icon-pre in MDL-36837 )
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks guys, i've integrated this now.

            Show
            poltawski Dan Poltawski added a comment - Thanks guys, i've integrated this now.
            Hide
            phalacee Jason Fowler added a comment -

            Tested in Firefox on Ubuntu and IE8 on Windows 7

            All icons appear to be working as expected

            Show
            phalacee Jason Fowler added a comment - Tested in Firefox on Ubuntu and IE8 on Windows 7 All icons appear to be working as expected
            Hide
            timhunt Tim Hunt added a comment -

            Just to note that the name of the partially correct icon was changed ot 'grade_partiallycorrect' before this was integrated. Thanks.

            Martin, $state->get_feedback_class() is question-specific code - just like the calling code, which is using that class, and is all about questions. Of course it is better for one bit of question code to call an existing question code method, rather than copy and paste the same if statement in three places.

            Anyway I have created MDL-36916 to tidy this up later.

            Show
            timhunt Tim Hunt added a comment - Just to note that the name of the partially correct icon was changed ot 'grade_partiallycorrect' before this was integrated. Thanks. Martin, $state->get_feedback_class() is question-specific code - just like the calling code, which is using that class, and is all about questions. Of course it is better for one bit of question code to call an existing question code method, rather than copy and paste the same if statement in three places. Anyway I have created MDL-36916 to tidy this up later.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Y E S !

            Closing as fixed, many thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12