Moodle
  1. Moodle
  2. MDL-31903

Course section highlight icon and title won't toggle

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      NOTE: should be tested on IE, FF, opera, chrome and any other browser you may have.

      1. Log in as admin
      2. Go to a course.
      3. Change the course format to topics and turn editing on
      4. Click the highlight icon (lightbulb) in section 1.
      5. Section should highlights and the icon and title ('Highlight this topic as the current topic') should change (icon might look same so look at page source and make sure it's changing)
      6. Hover over the highlight icon and it should display appropriate text.
      7. Now try the above with show/hide (eye) icon and make sure all works fine.
      Show
      NOTE: should be tested on IE, FF, opera, chrome and any other browser you may have. Log in as admin Go to a course. Change the course format to topics and turn editing on Click the highlight icon (lightbulb) in section 1. Section should highlights and the icon and title ('Highlight this topic as the current topic') should change (icon might look same so look at page source and make sure it's changing) Hover over the highlight icon and it should display appropriate text. Now try the above with show/hide (eye) icon and make sure all works fine.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-31903

      Description

      Section highlighting doesn't toggle the lightbulb icon and its title.

        Gliffy Diagrams

          Activity

          Show
          Juho Viitasalo added a comment - - edited Here is a patch: https://github.com/jiv-e/moodle/commit/ff9c91b6c5b260d517cd70a257664f4fc1d296d3
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks Juho, for reporting this bug and providing the patch.

          I think introducing new class may cause regression on various themes, hence it might be nice to have other ticket requesting for improvement.
          I have updated your patch with few minor things, like updating alt text and now it's up for peer-review.

          It was noticed that on section-hide icon we were wrongly introducing image title, hence removed in one extra commit.
          IMO title is not required and might be an issue for screen-readers (Sa we put image in div with proper title). Also found following:

          Please feel free to reject last commit if you feel image title is required.

          Show
          Rajesh Taneja added a comment - - edited Thanks Juho, for reporting this bug and providing the patch. I think introducing new class may cause regression on various themes, hence it might be nice to have other ticket requesting for improvement. I have updated your patch with few minor things, like updating alt text and now it's up for peer-review. It was noticed that on section-hide icon we were wrongly introducing image title, hence removed in one extra commit. IMO title is not required and might be an issue for screen-readers (Sa we put image in div with proper title). Also found following: Please feel free to reject last commit if you feel image title is required.
          Hide
          Juho Viitasalo added a comment -

          I don't understand what you mean by 'introducing new class'. Can you clarify that?

          I'm ok with removing the image title.

          Show
          Juho Viitasalo added a comment - I don't understand what you mean by 'introducing new class'. Can you clarify that? I'm ok with removing the image title.
          Show
          Rajesh Taneja added a comment - Thanks Juho, I meant https://github.com/rajeshtaneja/moodle/commit/0b1f9c28dc0ee1d84aca23556420ea1bb56a5b77#L2R96 and https://github.com/rajeshtaneja/moodle/commit/0b1f9c28dc0ee1d84aca23556420ea1bb56a5b77#L2R103 .
          Hide
          Adrian Greeve added a comment -

          The code looks good. The light bulk icon toggles on and off in editing mode. The tool tip pop up text also changes to match the condition.
          I did notice that switching editing on and off will result in the light bulb icon reverting to the old image. That might also be work investigating.

          Show
          Adrian Greeve added a comment - The code looks good. The light bulk icon toggles on and off in editing mode. The tool tip pop up text also changes to match the condition. I did notice that switching editing on and off will result in the light bulb icon reverting to the old image. That might also be work investigating.
          Hide
          Rajesh Taneja added a comment -

          Good Catch Adrian,

          I have fixed this. In addtion to that, I have fixed title in image tag for hightlight and hide/show. Moodle tends to add title attribute for ie only, so last commit fixes issue for FF.

          Show
          Rajesh Taneja added a comment - Good Catch Adrian, I have fixed this. In addtion to that, I have fixed title in image tag for hightlight and hide/show. Moodle tends to add title attribute for ie only, so last commit fixes issue for FF.
          Hide
          Juho Viitasalo added a comment -

          Thanks for taking this forward!

          Ok, I now understood that you meant a CSS classes. Yes it could be better to make that an improvement request, because it's not a bug and clearly a separate issue.

          Show
          Juho Viitasalo added a comment - Thanks for taking this forward! Ok, I now understood that you meant a CSS classes. Yes it could be better to make that an improvement request, because it's not a bug and clearly a separate issue.
          Hide
          Adrian Greeve added a comment -

          The code looks good. I tested it with IE, Firefox, Opera and Chrome. Everything works as it should.
          Thanks Raj.

          Show
          Adrian Greeve added a comment - The code looks good. I tested it with IE, Firefox, Opera and Chrome. Everything works as it should. Thanks Raj.
          Hide
          Rajesh Taneja added a comment -

          Thanks Adrian,

          Pushing it for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Adrian, Pushing it for integration review.
          Hide
          Sam Hemelryk 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
          Sam Hemelryk 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
          Rajesh Taneja added a comment -

          Thanks Sam,
          Branches re-based

          Show
          Rajesh Taneja added a comment - Thanks Sam, Branches re-based
          Hide
          Sam Hemelryk added a comment -

          Thanks guys this has been integrated now. In future could you please ensure all commit messages include the tracker issue number. In this case I have cherry-picked individual commits and edited where required.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks guys this has been integrated now. In future could you please ensure all commit messages include the tracker issue number. In this case I have cherry-picked individual commits and edited where required. Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          all good
          tested on ie,ff,opera and chromium..all good!
          Thanks

          Show
          Ankit Agarwal added a comment - all good tested on ie,ff,opera and chromium..all good! Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, finally! Yay!

          תודה רבה && شكرا جزيلا



          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, finally! Yay! תודה רבה && شكرا جزيلا Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: