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:
    • Rank:
      33555

      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

        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: