Moodle
  1. Moodle
  2. MDL-30879

Class name in description displayed after link omits space

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2, 2.3
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Course
    • Labels:
      None
    • Testing Instructions:
      Hide

      1. Add a URL: name 'My URL', description 'My fun description', tick the box 'Display description on course page', external URL 'http://www.google.com/'. Other settings default. Save and return to course.
      + URL displays with description below it
      2. Using Firebug or similar tool, examine the description ('My fun description').
      + Description is contained in div with class 'contentafterlink'.
      3. Click the eye button next to the URL.
      4. Repeat the test with Firebug.
      + Class of containing div should now be 'contentafterlink dimmed_text'.

      Before fixing this bug, the class is actually 'contentafterlinkdimmed_text', i.e. the space was omitted. This obviously causes any CSS that uses the class to fail.

      Show
      1. Add a URL: name 'My URL', description 'My fun description', tick the box 'Display description on course page', external URL 'http://www.google.com/'. Other settings default. Save and return to course. + URL displays with description below it 2. Using Firebug or similar tool, examine the description ('My fun description'). + Description is contained in div with class 'contentafterlink'. 3. Click the eye button next to the URL. 4. Repeat the test with Firebug. + Class of containing div should now be 'contentafterlink dimmed_text'. Before fixing this bug, the class is actually 'contentafterlinkdimmed_text', i.e. the space was omitted. This obviously causes any CSS that uses the class to fail.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-30879-master

      Description

      When you use the new feature (that I wrote) to include a description after activities on the course main page, in some circumstances the CSS class name can be corrupted in generated HTML. (See test script.)

        Gliffy Diagrams

          Activity

          Hide
          Sam Marshall added a comment -

          Trivial fix...

          Show
          Sam Marshall added a comment - Trivial fix...
          Hide
          Sam Marshall added a comment -

          (added fix for 2.1 branch as well)

          Show
          Sam Marshall added a comment - (added fix for 2.1 branch as well)
          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
          Sam Marshall added a comment -

          Rebased against MOODLE_21_STABLE, MOODLE_22_STABLE, and master.

          Show
          Sam Marshall added a comment - Rebased against MOODLE_21_STABLE, MOODLE_22_STABLE, and master.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam,

          while your change ensures the whitespace separation to happen, I'm not sure why you've decided to go with the "trim everything" route. In fact, it causes 2-spaces to remain there, if I'm not wrong. So I'd suggest to apply this change instead:

               // If specified, display extra content after link
               if ($content) {
          -        $contentpart = '<div class="contentafterlink' .
          +        $contentpart = '<div class="contentafterlink ' .
                           trim($textclasses) . '">' . $content . '</div>';
               }
          

          Looks simpler, avoids the double whitespace and ensures the separation. Or am I missing anything?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, while your change ensures the whitespace separation to happen, I'm not sure why you've decided to go with the "trim everything" route. In fact, it causes 2-spaces to remain there, if I'm not wrong. So I'd suggest to apply this change instead: // If specified, display extra content after link if ($content) { - $contentpart = '<div class="contentafterlink' . + $contentpart = '<div class="contentafterlink ' . trim($textclasses) . '">' . $content . '</div>'; } Looks simpler, avoids the double whitespace and ensures the separation. Or am I missing anything? Ciao
          Hide
          Sam Marshall added a comment -

          Your change (which is actually also what I did to start with) leaves an extra space in when there are no text classes:

          <div class="contentafterlink ">

          I thought this was ugly. But you're right, $textclasses is defined to include a space at the start of each class already, so my change has two spaces in the middle if there ARE text classes. Sigh! I've fixed it. I just verified new version in source view with both cases (with and without extra classes).

          Note, I also pushed all the branches I forgot to push, so the diff URLs work now.

          Show
          Sam Marshall added a comment - Your change (which is actually also what I did to start with) leaves an extra space in when there are no text classes: <div class="contentafterlink "> I thought this was ugly. But you're right, $textclasses is defined to include a space at the start of each class already, so my change has two spaces in the middle if there ARE text classes. Sigh! I've fixed it. I just verified new version in source view with both cases (with and without extra classes). Note, I also pushed all the branches I forgot to push, so the diff URLs work now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          LOL, combinations of 2 strings, 1 space and 1 trim

          Thanks, this has been integrated now. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - LOL, combinations of 2 strings, 1 space and 1 trim Thanks, this has been integrated now. Ciao
          Hide
          Michael de Raadt added a comment -

          Test result: passed Nice work guys!

          Show
          Michael de Raadt added a comment - Test result: passed Nice work guys!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The git and cvs repositories are happy receiving your very first contribution to Moodle for 2012. Happy new year!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The git and cvs repositories are happy receiving your very first contribution to Moodle for 2012. Happy new year! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: