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
    • 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

        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: