Details

    • Testing Instructions:
      Hide
      1. Select Clean theme
      2. In any browser use 'Print Preview' of the front page and observe that the link URL's are appended to the text.
      3. Apply the patch.
      4. In any browser use 'Print Preview' of the front page and observe that the link URL's are no longer appended to the text.
      Show
      Select Clean theme In any browser use 'Print Preview' of the front page and observe that the link URL's are appended to the text. Apply the patch. In any browser use 'Print Preview' of the front page and observe that the link URL's are no longer appended to the text.
    • Workaround:
      Hide

      Use the following CSS in the 'Clean' theme's custom CSS setting:

      @media print {
        a[href]:after {
          content: "";
        }
      }
      

      Show
      Use the following CSS in the 'Clean' theme's custom CSS setting: @media print { a[href]:after { content: ""; } }
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-40321_master

      Description

      The Bootstrap CSS pulls in some print styles that append the destination URL to links in the printout. This is helpful for printed content, but needs some kind of exception to stop it doing it for Moodle internal links.

      I added an exception for our institution by using this rule:

      a[href^="http://startofyourmoodle.url.com"]:after

      { content:""}

      But that doesn't generalise to any Moodle. Perhaps completely negating the print rule (which is in theme/bootstrapbase/less/reset.less) might be a good idea. The rule could then be re-instated but targetting only Moodle content in some way?

        Gliffy Diagrams

        1. mdl_40321_clean_1.png
          104 kB
        2. mdl_40321_clean_2.png
          85 kB
        3. mdl_40321_standard_1.png
          117 kB
        4. mdl_40321_standard_2.png
          126 kB
        5. mdl_40321_standard_3.png
          97 kB

          Issue Links

            Activity

            Hide
            Gareth J Barnard added a comment -

            I think the links might be a good idea, just perhaps with a tad smaller font.

            Show
            Gareth J Barnard added a comment - I think the links might be a good idea, just perhaps with a tad smaller font.
            Hide
            Mary Evans added a comment -

            This needs sorting out quick!

            Show
            Mary Evans added a comment - This needs sorting out quick!
            Hide
            Gareth J Barnard added a comment -

            Just had the thought that David Scotson's solution could be implemented in 'Clean' with CSS pre-processing - just like the original font serving mechanism:

              a[href^="[[theme:wwwroot]]"]:after {
                content: "";
              }
            

            in custom.css, then in lib.php:

            function theme_clean_set_wwwroot($css) {
                global $CFG;
                $tag = '[[theme:wwwroot]]';
                $css = str_replace($tag, $CFG->wwwroot, $css);
             
                return $css;
            }
            

            You get the idea...

            Not sure about 'https' though?

            Gareth

            Show
            Gareth J Barnard added a comment - Just had the thought that David Scotson 's solution could be implemented in 'Clean' with CSS pre-processing - just like the original font serving mechanism: a[href^="[[theme:wwwroot]]"]:after { content: ""; } in custom.css, then in lib.php: function theme_clean_set_wwwroot($css) { global $CFG; $tag = '[[theme:wwwroot]]'; $css = str_replace($tag, $CFG->wwwroot, $css);   return $css; } You get the idea... Not sure about 'https' though? Gareth
            Hide
            Gareth J Barnard added a comment -

            But, this might be overkill, as you could have a 'moodle/reset.less' with:

            @media print {
              a[href]:after {
                content: "";
              }
            }
            

            which would be included after 'bootstrap/reset.less', but it would mean that no links would be printed.

            Show
            Gareth J Barnard added a comment - But, this might be overkill, as you could have a 'moodle/reset.less' with: @media print { a[href]:after { content: ""; } } which would be included after 'bootstrap/reset.less', but it would mean that no links would be printed.
            Hide
            Mary Evans added a comment - - edited

            Just logging Greg's comments from MDL-40602

            https://tracker.moodle.org/browse/MDL-40602?focusedCommentId=240401&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-240401

            And his suggested fix...

            Change code in theme/bootstrapbase/less/bootstrap/reset.less from:
             

              
              a[href]:after {
                  content: " (" attr(href) ")";
              }
            

            to this:
             

              a[href]:after {
                  content: none;
              }
            
            

            Show
            Mary Evans added a comment - - edited Just logging Greg's comments from MDL-40602 https://tracker.moodle.org/browse/MDL-40602?focusedCommentId=240401&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-240401 And his suggested fix... Change code in theme/bootstrapbase/less/bootstrap/reset.less from:   a[href]:after { content: " (" attr(href) ")"; } to this:   a[href]:after { content: none; }
            Hide
            Mary Evans added a comment - - edited

            This makes sense as the above code is inside an @media print at line 167

            https://github.com/lazydaisy/moodle/blob/master/theme/bootstrapbase/less/bootstrap/reset.less#L167

            Show
            Mary Evans added a comment - - edited This makes sense as the above code is inside an @media print at line 167 https://github.com/lazydaisy/moodle/blob/master/theme/bootstrapbase/less/bootstrap/reset.less#L167
            Hide
            Gareth J Barnard added a comment -

            But I thought for maintainability reasons every 'less' file in the 'bootstrap' folder was untouchable?

            Show
            Gareth J Barnard added a comment - But I thought for maintainability reasons every 'less' file in the 'bootstrap' folder was untouchable?
            Hide
            Gareth J Barnard added a comment -

            Submitting for Peer Review of fix in Moodle 2.5 branch, if acceptable I'll create a 'master' version before requesting submission for integration.

            Show
            Gareth J Barnard added a comment - Submitting for Peer Review of fix in Moodle 2.5 branch, if acceptable I'll create a 'master' version before requesting submission for integration.
            Hide
            Mary Evans added a comment -

            OK...starting Peer Review

            Show
            Mary Evans added a comment - OK...starting Peer Review
            Hide
            Mary Evans added a comment -

            +1 for the code, it looks good and works OK, so submitting for Integration Review.

            Please do add a master branch that would be good.

            Thanks
            Mary

            Show
            Mary Evans added a comment - +1 for the code, it looks good and works OK, so submitting for Integration Review. Please do add a master branch that would be good. Thanks Mary
            Hide
            Gareth J Barnard added a comment - - edited

            Will do tomorrow. I've updated the compare as it was looking at my MOODLE_25_STABLE branch and not the Moodle repository one. And thanks for peer reviewing Mary Evans

            Show
            Gareth J Barnard added a comment - - edited Will do tomorrow. I've updated the compare as it was looking at my MOODLE_25_STABLE branch and not the Moodle repository one. And thanks for peer reviewing Mary Evans
            Hide
            Mary Evans added a comment - - edited

            @Gareth, The compare should be with your repository's updated MOODLE_25_STABLE branch, this should be the same branch that you based your patch on. If this is not the case then I will have to reopen this until your branches are both uptodate and based on their respective branches.

            Show
            Mary Evans added a comment - - edited @Gareth, The compare should be with your repository's updated MOODLE_25_STABLE branch, this should be the same branch that you based your patch on. If this is not the case then I will have to reopen this until your branches are both uptodate and based on their respective branches.
            Hide
            Mary Evans added a comment -
            Show
            Mary Evans added a comment - You should write it like this... https://github.com/gjb2048/moodle/compare/MOODLE_25_STABLE...wip-MDL-40321_M25
            Hide
            Gareth J Barnard added a comment -

            They are up to date

            Show
            Gareth J Barnard added a comment - They are up to date
            Hide
            Mary Evans added a comment -

            I can see that I was just wondering why you changed the link

            Show
            Mary Evans added a comment - I can see that I was just wondering why you changed the link
            Hide
            Gareth J Barnard added a comment -

            Because locally I had based the wip-MDL-40312_M25 branch upon a latest MOODLE_25_BRANCH but had only pushed the former remotely and not the latter so the compare was wrong. So I thought the compare URL was incorrect. I've since pushed MOODLE_25_STABLE and hence the corrected URL compare is correct.

            Show
            Gareth J Barnard added a comment - Because locally I had based the wip- MDL-40312 _M25 branch upon a latest MOODLE_25_BRANCH but had only pushed the former remotely and not the latter so the compare was wrong. So I thought the compare URL was incorrect. I've since pushed MOODLE_25_STABLE and hence the corrected URL compare is correct.
            Hide
            Damyon Wiese added a comment -

            Thanks Gareth,

            This has been integrated to 25 and master now.

            Note - I added a commit to keep the less files tidy - we recently added a bootstrapoverrides.less file which has the same purpose as your reset.less file - so I moved your rules into that one and got rid of reset.less.

            The bootstrapoverrides.less file was not backported to 25 - but I added it on that branch anyway to keep the branches sort of in sync.

            Show
            Damyon Wiese added a comment - Thanks Gareth, This has been integrated to 25 and master now. Note - I added a commit to keep the less files tidy - we recently added a bootstrapoverrides.less file which has the same purpose as your reset.less file - so I moved your rules into that one and got rid of reset.less. The bootstrapoverrides.less file was not backported to 25 - but I added it on that branch anyway to keep the branches sort of in sync.
            Hide
            Gareth J Barnard added a comment -

            Dear Damyon Wiese,

            No problem and thanks .

            I named it 'reset.less' to be consistent with the Bootstrap names.

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - Dear Damyon Wiese , No problem and thanks . I named it 'reset.less' to be consistent with the Bootstrap names. Cheers, Gareth
            Hide
            Jérôme Mouneyrac added a comment -

            On the print preview (linux/chrome), with the Standard theme, we can see the texts as blue link. On the Clean theme we don't know anymore that these texts are links. I prefer the look with links but it removes the information of the text been a link. The second issue is that the themes are not consistent too. Personally I have no problem with that, but just to be sure I fail it to know the opinion of integrator. You can pass it straight away if it's not a problem.

            Show
            Jérôme Mouneyrac added a comment - On the print preview (linux/chrome), with the Standard theme, we can see the texts as blue link. On the Clean theme we don't know anymore that these texts are links. I prefer the look with links but it removes the information of the text been a link. The second issue is that the themes are not consistent too. Personally I have no problem with that, but just to be sure I fail it to know the opinion of integrator. You can pass it straight away if it's not a problem.
            Hide
            Damyon Wiese added a comment -

            Good point Jerome - I think colouring the links for the print style sheet is a good idea.

            Show
            Damyon Wiese added a comment - Good point Jerome - I think colouring the links for the print style sheet is a good idea.
            Hide
            Mary Evans added a comment -

            Damyon/Jerome,

            Whilst I agree about making a link distinguishable when printing, I disagree about colour, bold maybe, but colour no. I only say this as I always print in black and white to save on colored ink.

            If you notice also that some themes do not specify a:link/a:visited these they leave to the browser default, which in most cases is blue/red mine is bright pink/yellow so they stand out while theming, as I prefer to style them to match the theme.

            So my -1 for colour but am happier if links are bold.

            Thank you.
            Mary

            Show
            Mary Evans added a comment - Damyon/Jerome, Whilst I agree about making a link distinguishable when printing, I disagree about colour, bold maybe, but colour no. I only say this as I always print in black and white to save on colored ink. If you notice also that some themes do not specify a:link/a:visited these they leave to the browser default, which in most cases is blue/red mine is bright pink/yellow so they stand out while theming, as I prefer to style them to match the theme. So my -1 for colour but am happier if links are bold . Thank you. Mary
            Hide
            Gareth J Barnard added a comment -

            Ok, regardless of what happens at the moment, given what has been said I consider that the links should be coloured as you can always choose to print in black and white, bold uses more ink if you do and internal links should not be printed as they are pointless and the real reason the issue was raised in the first place. So later today I'll consider how the CSS pre-processing mechanism above could be employed. Tips on the https issue appreciated.

            Show
            Gareth J Barnard added a comment - Ok, regardless of what happens at the moment, given what has been said I consider that the links should be coloured as you can always choose to print in black and white, bold uses more ink if you do and internal links should not be printed as they are pointless and the real reason the issue was raised in the first place. So later today I'll consider how the CSS pre-processing mechanism above could be employed. Tips on the https issue appreciated.
            Hide
            Gareth J Barnard added a comment -

            And what I've said above could apply to all themes.

            Show
            Gareth J Barnard added a comment - And what I've said above could apply to all themes.
            Hide
            Gareth J Barnard added a comment - - edited

            Ok, I've done some investigation and attached some images showing links in the Standard and Clean themes without the patch in Moodle 2.5. It appears that by default the links in the Standard theme are not printed as you say Jerome - I'm using Chrome Version '29.0.1547.57 m' on windows - looking at the screen shots explictly printed links are there in both themes (this patch removes the horrible extra bracketed ones), it's just in the Clean theme they are not blue (see the mdl_40321_standard_3.png and mdl_40321_clean_1.png screen shots). Therefore I believe you are referring to a different styling issue and not what this patch addresses. Therefore the fix as stands is consistent with the current base based themes and therefore anything else would be an improvement. Therefore I believe that the existing patch should stand. If not then I consider that internal links should not be printed as shown by the 'Clean' screen shots.

            Show
            Gareth J Barnard added a comment - - edited Ok, I've done some investigation and attached some images showing links in the Standard and Clean themes without the patch in Moodle 2.5. It appears that by default the links in the Standard theme are not printed as you say Jerome - I'm using Chrome Version '29.0.1547.57 m' on windows - looking at the screen shots explictly printed links are there in both themes (this patch removes the horrible extra bracketed ones), it's just in the Clean theme they are not blue (see the mdl_40321_standard_3.png and mdl_40321_clean_1.png screen shots). Therefore I believe you are referring to a different styling issue and not what this patch addresses. Therefore the fix as stands is consistent with the current base based themes and therefore anything else would be an improvement. Therefore I believe that the existing patch should stand. If not then I consider that internal links should not be printed as shown by the 'Clean' screen shots.
            Hide
            Damyon Wiese added a comment -

            I've added a new issue for the missing link text decoration in bootstrap.

            Jerome - can you pass this if you are satisfied (back to testing)?

            Show
            Damyon Wiese added a comment - I've added a new issue for the missing link text decoration in bootstrap. Jerome - can you pass this if you are satisfied (back to testing)?
            Hide
            Jérôme Mouneyrac added a comment -

            As Damyon created an issue for the missing blue coloration, passing Thanks guy.

            Show
            Jérôme Mouneyrac added a comment - As Damyon created an issue for the missing blue coloration, passing Thanks guy.
            Hide
            Damyon Wiese added a comment -

            There was a young man named McGee
            Who thought squashing bugs was easy
            He tried it one day
            And to his dismay
            The bug guts made his keyboard all greasy

            Thanks!

            This has issue has been fixed and released in Moodle.

            Show
            Damyon Wiese added a comment - There was a young man named McGee Who thought squashing bugs was easy He tried it one day And to his dismay The bug guts made his keyboard all greasy Thanks! This has issue has been fixed and released in Moodle.

              People

              • Votes:
                7 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: