Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30879

Class name in description displayed after link omits space

          Details

          • Type: Bug
          • Status: Closed
          • Priority: 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

                Attachments

                  Activity

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

                  Trivial fix...

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

                  (added fix for 2.1 branch as well)

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

                  Rebased against MOODLE_21_STABLE, MOODLE_22_STABLE, and master.

                  Show
                  quen Sam Marshall added a comment - Rebased against MOODLE_21_STABLE, MOODLE_22_STABLE, and master.
                  Hide
                  Permalink
                  stronk7 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
                  stronk7 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
                  quen 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
                  quen 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
                  stronk7 Eloy Lafuente (stronk7) added a comment -

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

                  Thanks, this has been integrated now. Ciao

                  Show
                  stronk7 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
                  salvetore Michael de Raadt added a comment -

                  Test result: passed Nice work guys!

                  Show
                  salvetore Michael de Raadt added a comment - Test result: passed Nice work guys!
                  Hide
                  Permalink
                  stronk7 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
                  stronk7 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:
                        Fix Release Date:
                        9/Jan/12