Moodle
  1. Moodle
  2. MDL-30714

Afterburner Logo does not link to the site homepage

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.2
    • Component/s: Themes, Usability
    • Labels:
    • Environment:
      Moodle.net QA Testing Site
    • Testing Instructions:
      Hide
      1. Select Afterburner theme
      2. When hovering over the Logo the user should see...
        --the cursor change to a 'Hand'
        --tool-tip 'Home'
        --the URL of the current home page for the site
      3. Test by CLICKING the Logo link takes you to the site Home page
      Show
      Select Afterburner theme When hovering over the Logo the user should see... --the cursor change to a 'Hand' --tool-tip 'Home' --the URL of the current home page for the site Test by CLICKING the Logo link takes you to the site Home page
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      In the 1.9 Afterburner theme the logo links to the site homepage.
      http://www.newschoollearning.com/moodle/?&theme=afterburner

      In 2.2 the logo does not link to the homepage.
      http://qa.moodle.net/?theme=afterburner

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting that.

            I've put that on the backlog.

            In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

            Show
            Michael de Raadt added a comment - Thanks for reporting that. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
            Hide
            John Bracegirdle added a comment -

            This is my first ever patch, and I'm using Windows 7 on my laptop.

            Show
            John Bracegirdle added a comment - This is my first ever patch, and I'm using Windows 7 on my laptop.
            Hide
            Sam Hemelryk added a comment -

            Hi guys,
            Sending this back sorry, this change breaks XHTML strict in that you can't put a div inside an a.
            My suggestion would be to either change the div to an a and add CSS to change its display type to block (so that #logo becomes an a) OR change the div to a span and force display on that, although I think the first option would be the cleaner.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi guys, Sending this back sorry, this change breaks XHTML strict in that you can't put a div inside an a. My suggestion would be to either change the div to an a and add CSS to change its display type to block (so that #logo becomes an a) OR change the div to a span and force display on that, although I think the first option would be the cleaner. Cheers Sam
            Hide
            Mary Evans added a comment -

            Will fix later...cheers

            Show
            Mary Evans added a comment - Will fix later...cheers
            Hide
            Mary Evans added a comment - - edited

            The patch isn't working.

            All I get is a blank space where the logo should be.

            I'm wondering if this is something to do with the fact this is declared as a CSS function as originally it was set as a background.

            So perhaps if I delete the CSS function in afterburner/lib.php and add the following to the top of afterburner/layout/default.php...

            $haslogo = (!empty($PAGE->theme->settings->logo));
             
            if (!empty($PAGE->theme->settings->logo)) {
                $logo = $PAGE->theme->settings->logo;
            } else {
                $logo = $OUTPUT->pix_url('images/logo', 'theme');
            }
            

            and then simply add

            <div id="logo">
                <a title="<?php echo get_string('home');?>" href="<?php echo $CFG->wwwroot?>"><img src="<?php echo $logo; ?>" alt="<?php echo get_string('logo','theme_afterburner');?>" /></a>
            </div>
            

            I'll test this later

            Cheers
            Mary

            Show
            Mary Evans added a comment - - edited The patch isn't working. All I get is a blank space where the logo should be. I'm wondering if this is something to do with the fact this is declared as a CSS function as originally it was set as a background. So perhaps if I delete the CSS function in afterburner/lib.php and add the following to the top of afterburner/layout/default.php... $haslogo = (!empty($PAGE->theme->settings->logo));   if (!empty($PAGE->theme->settings->logo)) { $logo = $PAGE->theme->settings->logo; } else { $logo = $OUTPUT->pix_url('images/logo', 'theme'); } and then simply add <div id="logo"> <a title="<?php echo get_string('home');?>" href="<?php echo $CFG->wwwroot?>"><img src="<?php echo $logo; ?>" alt="<?php echo get_string('logo','theme_afterburner');?>" /></a> </div> I'll test this later Cheers Mary
            Hide
            Sam Hemelryk added a comment -

            Hi Mary,

            Probably the one advantage to displaying the logo using CSS is that the browser imposing heavy caching on it unless explicitly told not to by the server.
            Changing to an inline img tag as you've done is probably going to make things simpler in the long and will hopefully solve this issue nicely. Certainly its a win for accessibility and usability of a site.
            The only downside is that once again it will be the responsibility of the server admin to make sure that the location they are serving the image from has been properly set up to serve the image with caching headers. Not a biggie really and perhaps we just need to improve the help string for the logo setting to mention something along those lines (it should also mention that to avoid caching problems when changing the logo image you should always give it a different name).

            Anyway certainly if using the inline image works it gets my +1

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Mary, Probably the one advantage to displaying the logo using CSS is that the browser imposing heavy caching on it unless explicitly told not to by the server. Changing to an inline img tag as you've done is probably going to make things simpler in the long and will hopefully solve this issue nicely. Certainly its a win for accessibility and usability of a site. The only downside is that once again it will be the responsibility of the server admin to make sure that the location they are serving the image from has been properly set up to serve the image with caching headers. Not a biggie really and perhaps we just need to improve the help string for the logo setting to mention something along those lines (it should also mention that to avoid caching problems when changing the logo image you should always give it a different name). Anyway certainly if using the inline image works it gets my +1 Cheers Sam
            Hide
            Mary Evans added a comment -

            Hi Sam,
            This is why I kept it the way I did it...however I have found a fix based on your earlier comments.
            Thanks
            Mary

            Show
            Mary Evans added a comment - Hi Sam, This is why I kept it the way I did it...however I have found a fix based on your earlier comments. Thanks Mary
            Hide
            Mary Evans added a comment -

            On the last minute again!
            Hope this is not too late!

            Show
            Mary Evans added a comment - On the last minute again! Hope this is not too late!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            LOL, Mary.

            Am I missing anything or your patch is clearly missing to output the information? Checking...

            Show
            Eloy Lafuente (stronk7) added a comment - LOL, Mary. Am I missing anything or your patch is clearly missing to output the information? Checking...
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Yeah, it was missing one echo for the $CFG->wwwroot and changed get_string() by print_string(). I've added one extra commit fixing that. Funny!

            Offtopic: While looking to this, I've noticed that there are various themes using some harcoded strings, like "Home" (arialist, magazine...). Surely all them should be reviewed and converted to proper lang strings. If this issue does not sound to you, feel free to create a new one for it, TIA!

            Offtopic2: Also, not sure in which status your branches were... because when I tried the initial merge, I got dozens of commits that already were upstream, but with different commit-ids. Seems that you had rebased incorrectly or so. Luckily, I detected it and integrated your issue by cherry-picking the exact commit. Just be warned, something in your git-flow seems to be incorrect.

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Yeah, it was missing one echo for the $CFG->wwwroot and changed get_string() by print_string(). I've added one extra commit fixing that. Funny! Offtopic: While looking to this, I've noticed that there are various themes using some harcoded strings, like "Home" (arialist, magazine...). Surely all them should be reviewed and converted to proper lang strings. If this issue does not sound to you, feel free to create a new one for it, TIA! Offtopic2: Also, not sure in which status your branches were... because when I tried the initial merge, I got dozens of commits that already were upstream, but with different commit-ids. Seems that you had rebased incorrectly or so. Luckily, I detected it and integrated your issue by cherry-picking the exact commit. Just be warned, something in your git-flow seems to be incorrect.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            Mary Evans added a comment -

            Currently...but not sure what has caused this...but when I do "git checkout master" it tells me that my branch and origin/master have diverged with 59 and 98 commits respectively.

            So..."Wise One" how do I get out of the MESS!!!

            Mary

            Show
            Mary Evans added a comment - Currently...but not sure what has caused this...but when I do "git checkout master" it tells me that my branch and origin/master have diverged with 59 and 98 commits respectively. So..."Wise One" how do I get out of the MESS!!! Mary
            Hide
            Rossiani Wijaya added a comment -

            This is working great.

            Test passed.

            Show
            Rossiani Wijaya added a comment - This is working great. Test passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: