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

      Description

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

        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: