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

          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.)

            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: