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 from Repository:
        • Pull Master Branch:
          MDL-30879-master
        • Pull Master Diff URL:

          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

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              Sam Marshall added a comment -

              Trivial fix...

              Show
              Sam Marshall added a comment - Trivial fix...
              Hide
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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: