Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Gradebook, Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Create course with some hidden and visible graded activities (i.e. assignments)
      2. Open the grader report, make sure all columns are in place and hidden activities names are dimmed

      Test in Standard, Clean, MyMobile and some other themes

      Show
      Create course with some hidden and visible graded activities (i.e. assignments) Open the grader report, make sure all columns are in place and hidden activities names are dimmed Test in Standard, Clean, MyMobile and some other themes
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-39741-master

      Description

      See screenshots. There are two assignments, as1 and as2. as1 has been hidden on the course page. In standard it is visible but greyed out (which is correct).

      In Clean the assignment header is missing leading the rest of the columns to be out of alignment. Also the hidden assignment cell should contain the student's grade.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            bawjaws David Scotson added a comment -

            Probably (not looked at the HTML so just guessing) the classname .hidden is used. There were a few issues like that. In Bootstrap it's used to actually hide things.

            There's a (very incomplete) META bug to track these issues here:
            https://tracker.moodle.org/browse/MDL-38081

            And the current similar workarounds are mostly in this file:

            https://github.com/moodle/moodle/blob/master/theme/bootstrapbase/less/moodle/undo.less#L56-L61

            Show
            bawjaws David Scotson added a comment - Probably (not looked at the HTML so just guessing) the classname .hidden is used. There were a few issues like that. In Bootstrap it's used to actually hide things. There's a (very incomplete) META bug to track these issues here: https://tracker.moodle.org/browse/MDL-38081 And the current similar workarounds are mostly in this file: https://github.com/moodle/moodle/blob/master/theme/bootstrapbase/less/moodle/undo.less#L56-L61
            Hide
            marina Marina Glancy added a comment -

            Just to note that when section or module is hidden on course view page the links have class .dimmed and the spans with text have class .dimmed_text
            The best solution would be to use those/similar class names in grader report as well. But the question is whether we can change it in 2.5 because it's already "stable"

            Show
            marina Marina Glancy added a comment - Just to note that when section or module is hidden on course view page the links have class .dimmed and the spans with text have class .dimmed_text The best solution would be to use those/similar class names in grader report as well. But the question is whether we can change it in 2.5 because it's already "stable"
            Hide
            lazydaisy Mary Evans added a comment -

            There is nothing stopping us fixing this in bootstrapbase by adding that missing CSS if it is missing. If .hiden is the problem then we can target that with css using the .pagelayout-report .activity .hidden

            { blah blah blah}

            or something similar.

            Like I have said previously why should we bang our heads on the wall when it is simple to change how Bootstrapbase works in Moodle. I don't see any benefit making Moodle change to suit every new framework.

            Show
            lazydaisy Mary Evans added a comment - There is nothing stopping us fixing this in bootstrapbase by adding that missing CSS if it is missing. If .hiden is the problem then we can target that with css using the .pagelayout-report .activity .hidden { blah blah blah} or something similar. Like I have said previously why should we bang our heads on the wall when it is simple to change how Bootstrapbase works in Moodle. I don't see any benefit making Moodle change to suit every new framework.
            Hide
            marina Marina Glancy added a comment -

            Mary, I agree and disagree. In this case the choice of class name is not exactly correct and bootstrap theme just helped us to find it. When displaying the activity inside the course we use the class .dimmed. Why should we use another class name in the gradebook?

            Show
            marina Marina Glancy added a comment - Mary, I agree and disagree. In this case the choice of class name is not exactly correct and bootstrap theme just helped us to find it. When displaying the activity inside the course we use the class .dimmed. Why should we use another class name in the gradebook?
            Hide
            marina Marina Glancy added a comment -

            Requesting peer review

            Show
            marina Marina Glancy added a comment - Requesting peer review
            Hide
            marina Marina Glancy added a comment -

            I also need to remove this from bootstrapbase, will do tomorrow:

            table#user-grades .hidden,
            table#user-grades .hidden a {
                .muted
            }

            Show
            marina Marina Glancy added a comment - I also need to remove this from bootstrapbase, will do tomorrow: table#user-grades .hidden, table#user-grades .hidden a { .muted }
            Hide
            lazydaisy Mary Evans added a comment -

            I see the changes you made and these look to be the better option. However, I still cannot get my head round how bootstrapbase hide those hidden element for this particular part of the gradebook as the css for it in bootstrapbase/less/moodle/grade.less was correct by giving it the equivalent of a 'dimmed-text' or 'muted' as used in Bootstrap which is '@lightgrey' rather than completely hidden.

             
            table#user-grades .hidden,
            table#user-grades .hidden a {
                .muted
            }
             

            So should you not be removing this CSS too?

            The fact Bootstrapbase changed this to muted but then went an hide it too seems odd but understandable given that the class was wrong in the first place. Si in that respect I take back what I said in part.

            Perhaps what we should be thinking about is overhauling areas like this that have their own CSS and making those style.css into .less files in core and compiling them into base theme. That way bootstrapbase would not need to have all those common styles, only those pertinent to bootstrap styling conventions, so there would be no need to 'exclude' the grader stylesheet in bootstrapbase.

            I can see this idea getting everyone in a dither!

            Show
            lazydaisy Mary Evans added a comment - I see the changes you made and these look to be the better option. However, I still cannot get my head round how bootstrapbase hide those hidden element for this particular part of the gradebook as the css for it in bootstrapbase/less/moodle/grade.less was correct by giving it the equivalent of a 'dimmed-text' or 'muted' as used in Bootstrap which is '@lightgrey' rather than completely hidden.   table#user-grades .hidden, table#user-grades .hidden a { .muted }   So should you not be removing this CSS too? The fact Bootstrapbase changed this to muted but then went an hide it too seems odd but understandable given that the class was wrong in the first place. Si in that respect I take back what I said in part. Perhaps what we should be thinking about is overhauling areas like this that have their own CSS and making those style.css into .less files in core and compiling them into base theme. That way bootstrapbase would not need to have all those common styles, only those pertinent to bootstrap styling conventions, so there would be no need to 'exclude' the grader stylesheet in bootstrapbase. I can see this idea getting everyone in a dither!
            Hide
            lazydaisy Mary Evans added a comment -

            I didn't see your last comment as I was still writing this and testing at the same time. I am a slow writer!

            Show
            lazydaisy Mary Evans added a comment - I didn't see your last comment as I was still writing this and testing at the same time. I am a slow writer!
            Hide
            bawjaws David Scotson added a comment -

            The .muted line is there because I made mostly mechanical translations of the various assorted colors (mostly shades of red, green and gray, though only some of the gray had clear semantic purposes like muted text) to the Bootstrap equivalent (e.g. color: @successText or .muted) when porting the Base CSS to less.

            I agree with Marina that standardising on one classname for dimmed text is the best solution. And I'd support backporting it too since it's such a small focused change.

            Show
            bawjaws David Scotson added a comment - The .muted line is there because I made mostly mechanical translations of the various assorted colors (mostly shades of red, green and gray, though only some of the gray had clear semantic purposes like muted text) to the Bootstrap equivalent (e.g. color: @successText or .muted) when porting the Base CSS to less. I agree with Marina that standardising on one classname for dimmed text is the best solution. And I'd support backporting it too since it's such a small focused change.
            Hide
            marina Marina Glancy added a comment -

            I added commit removing unused lines from bootstrap

            Show
            marina Marina Glancy added a comment - I added commit removing unused lines from bootstrap
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Marina,

            The patch looks good and works great.

            Just wondering why the dimmed_text attribute needs to be added in myMobile theme? Why can't it used from core.css like the rest?

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Marina, The patch looks good and works great. Just wondering why the dimmed_text attribute needs to be added in myMobile theme? Why can't it used from core.css like the rest?
            Hide
            marina Marina Glancy added a comment -

            Hi Rossie, thanks for review. MyMobile specifies the link color using CSS element id and it has precedence. Therefore I have to overwrite with specifying the element id as well

            Show
            marina Marina Glancy added a comment - Hi Rossie, thanks for review. MyMobile specifies the link color using CSS element id and it has precedence. Therefore I have to overwrite with specifying the element id as well
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Marina, this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Marina, this has been integrated now
            Hide
            andyjdavis Andrew Davis added a comment -

            This seems fine although I ran into MDL-40131 while testing. Passing.

            Show
            andyjdavis Andrew Davis added a comment - This seems fine although I ran into MDL-40131 while testing. Passing.
            Hide
            damyon Damyon Wiese added a comment -

            This issue is fixed! Hurray! Hurray!
            Your issue is fixed, what a wonderful day!

            Cheers, Damyon

            Show
            damyon Damyon Wiese added a comment - This issue is fixed! Hurray! Hurray! Your issue is fixed, what a wonderful day! Cheers, Damyon

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13