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

Hide "add question to activity" button using javascript

    Details

    • Testing Instructions:
      Hide
      1. enable the activity feedback
      2. disable javascript
      3. create a new feedback instance
      4. go to the "Edit questions" page. There should be the "Add question to activity" button
      5. enable javascript
      6. reload this page. The button should be gone.
      Show
      enable the activity feedback disable javascript create a new feedback instance go to the "Edit questions" page. There should be the "Add question to activity" button enable javascript reload this page. The button should be gone.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38079_master

      Description

      Replication steps:
      Assuming you have javascript turned on

      1. Create a course
      2. Add the feedback activity to the course
      3. Edit questions
      4. Select a question type from the drop down menu

      Expected: Item gets selected, click on "Add question to activity" to add activity

      Actual: Item gets selected and automatically added to the activity

      This makes the button redundant for browsers with javascript turned on and is a source for potential confusion. I understand why we need the button there when javascript is not enabled in the browser, is there any other consideration I'm missing here?

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for reporting that, Marcus.

              This issue has been reported before, but it was closed because it became inactive. I've linked the previous issue and I will leave this issue open, as it has more detail.

              If you can propose a code solution, that will help others who may have the same need and will increase the chance of this improvement coming about sooner. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

              Show
              salvetore Michael de Raadt added a comment - Thanks for reporting that, Marcus. This issue has been reported before, but it was closed because it became inactive. I've linked the previous issue and I will leave this issue open, as it has more detail. If you can propose a code solution, that will help others who may have the same need and will increase the chance of this improvement coming about sooner. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
              Hide
              mr-russ Russell Smith added a comment -

              Patch to hide the redundant button in the feedback module.

              I've attempted to follow the example guidelines at http://docs.moodle.org/dev/JavaScript_guidelines as I've not contributed a javascript patch as yet.

              Any feedback would be appreciated.

              Show
              mr-russ Russell Smith added a comment - Patch to hide the redundant button in the feedback module. I've attempted to follow the example guidelines at http://docs.moodle.org/dev/JavaScript_guidelines as I've not contributed a javascript patch as yet. Any feedback would be appreciated.
              Hide
              mr-russ Russell Smith added a comment -

              Michael has asked the 'patch' label get added, is it possible for me to do that as I didn't report the bug?

              Show
              mr-russ Russell Smith added a comment - Michael has asked the 'patch' label get added, is it possible for me to do that as I didn't report the bug?
              Hide
              mr-russ Russell Smith added a comment -

              Hi,

              A patch has been attached to this for six weeks. How do you progress this through to the next stage. Is there a better way to get to the point of items being code-reviewed?

              Thanks

              Russell

              Show
              mr-russ Russell Smith added a comment - Hi, A patch has been attached to this for six weeks. How do you progress this through to the next stage. Is there a better way to get to the point of items being code-reviewed? Thanks Russell
              Hide
              mr-russ Russell Smith added a comment -

              A patch for 2.3 is complete, I'm now working on got branches for 2.4 and master. I'll upload those when I've sorted out the best way to publish those branches.

              Show
              mr-russ Russell Smith added a comment - A patch for 2.3 is complete, I'm now working on got branches for 2.4 and master. I'll upload those when I've sorted out the best way to publish those branches.
              Hide
              grabs Andreas Grabs added a comment -

              Hi Russell,
              I am sorry, that you had to wait for this long time. I was really busy and this issue was out of my mind.
              Thank you very much for this patch.
              Best regards
              Andreas

              Show
              grabs Andreas Grabs added a comment - Hi Russell, I am sorry, that you had to wait for this long time. I was really busy and this issue was out of my mind. Thank you very much for this patch. Best regards Andreas
              Hide
              poltawski Dan Poltawski 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
              poltawski Dan Poltawski 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
              damyon Damyon Wiese added a comment -

              Thanks for working on this.

              I think this patch just needs a few changes before it is considered. Also this is an improvement, and we are in freeze so it will get integration_held when it is ready for integration.

              Changes:
              1. This is not the way to add attributes with addElement.

              $mform->addElement('submit', 'add_item', get_string('add_item', 'feedback'), 'class="submitbutton"');

              Use
              $mform->addElement('submit', 'add_item', get_string('add_item', 'feedback'), array('class'=>'submitbutton'));

              2. The css is very generic - if you added anything with the submitbutton class anywhere in the feedback module later it would clash with this rule. I recommend making the class more specific (addrulesubmitbutton) and the rule more specific (start with #page-mod-feedback-edit at least).

              Cheers!

              Show
              damyon Damyon Wiese added a comment - Thanks for working on this. I think this patch just needs a few changes before it is considered. Also this is an improvement, and we are in freeze so it will get integration_held when it is ready for integration. Changes: 1. This is not the way to add attributes with addElement. $mform->addElement('submit', 'add_item', get_string('add_item', 'feedback'), 'class="submitbutton"'); Use $mform->addElement('submit', 'add_item', get_string('add_item', 'feedback'), array('class'=>'submitbutton')); 2. The css is very generic - if you added anything with the submitbutton class anywhere in the feedback module later it would clash with this rule. I recommend making the class more specific (addrulesubmitbutton) and the rule more specific (start with #page-mod-feedback-edit at least). Cheers!
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              grabs Andreas Grabs added a comment -

              Hi Damyon,
              thank you for the hints! It should be ok now.
              Best regards an a nice weekend
              Andreas

              Show
              grabs Andreas Grabs added a comment - Hi Damyon, thank you for the hints! It should be ok now. Best regards an a nice weekend Andreas
              Hide
              damyon Damyon Wiese added a comment -

              Thanks Andreas, looks good now. Adding integration_held label.

              Show
              damyon Damyon Wiese added a comment - Thanks Andreas, looks good now. Adding integration_held label.
              Hide
              mr-russ Russell Smith added a comment -

              Now 2.5 is out, what is the usual wait for integration held issues to be processed?

              Thanks

              Russell

              Show
              mr-russ Russell Smith added a comment - Now 2.5 is out, what is the usual wait for integration held issues to be processed? Thanks Russell
              Hide
              marina Marina Glancy added a comment -

              Is there some particular reason not to use class "hiddenifjs" here?

              Show
              marina Marina Glancy added a comment - Is there some particular reason not to use class "hiddenifjs" here?
              Hide
              mr-russ Russell Smith added a comment -

              Mostly because the documentation I used to build the patch doesn't mention it; http://docs.moodle.org/dev/JavaScript_guidelines

              Should this be docs required if that is the appropriate method of hiding js items?

              Show
              mr-russ Russell Smith added a comment - Mostly because the documentation I used to build the patch doesn't mention it; http://docs.moodle.org/dev/JavaScript_guidelines Should this be docs required if that is the appropriate method of hiding js items?
              Hide
              marina Marina Glancy added a comment -

              nice! Now with your help Russell we'll find the gaps in documentation

              This is when it was introduced:
              https://github.com/moodle/moodle/commit/2d65597b6c7b8d67be6ffbba4d85d16befaf216b

              Andrew Nicols do you think we can suggest using css classes 'hiddenifjs' and 'visibleifjs' in JS guidelines? http://docs.moodle.org/dev/JavaScript_guidelines#Different_content_when_JavaScript_is_on_or_off

              Anyway, Russell, can you please change your branches to use this class? Thanks

              Show
              marina Marina Glancy added a comment - nice! Now with your help Russell we'll find the gaps in documentation This is when it was introduced: https://github.com/moodle/moodle/commit/2d65597b6c7b8d67be6ffbba4d85d16befaf216b Andrew Nicols do you think we can suggest using css classes 'hiddenifjs' and 'visibleifjs' in JS guidelines? http://docs.moodle.org/dev/JavaScript_guidelines#Different_content_when_JavaScript_is_on_or_off Anyway, Russell, can you please change your branches to use this class? Thanks
              Hide
              mr-russ Russell Smith added a comment -

              I've redone all the branches to use hiddenifjs as requested. They are now in my name instead of Andreas'. I checked each of the branches to confirm hiddenifjs was an acceptable class. 2.3 is the oldest and is newer than the 3 year old commit that added this support.

              Show
              mr-russ Russell Smith added a comment - I've redone all the branches to use hiddenifjs as requested. They are now in my name instead of Andreas'. I checked each of the branches to confirm hiddenifjs was an acceptable class. 2.3 is the oldest and is newer than the 3 year old commit that added this support.
              Hide
              marina Marina Glancy added a comment -

              Thanks, integrated in 2.3, 2.4, 2.5 and master

              Show
              marina Marina Glancy added a comment - Thanks, integrated in 2.3, 2.4, 2.5 and master
              Hide
              dmonllao David Monllaó added a comment -

              Tested in 23, 24, 25 and master branches, it passes

              Show
              dmonllao David Monllaó added a comment - Tested in 23, 24, 25 and master branches, it passes
              Hide
              mr-russ Russell Smith added a comment -

              To repeat Marina's comment since I added Andrew;

              Andrew Nicols do you think we can suggest using css classes 'hiddenifjs' and 'visibleifjs' in JS guidelines? http://docs.moodle.org/dev/JavaScript_guidelines#Different_content_when_JavaScript_is_on_or_off

              Should I add 'docs required' so this doesn't get closed without alteration to that page?

              Show
              mr-russ Russell Smith added a comment - To repeat Marina's comment since I added Andrew; Andrew Nicols do you think we can suggest using css classes 'hiddenifjs' and 'visibleifjs' in JS guidelines? http://docs.moodle.org/dev/JavaScript_guidelines#Different_content_when_JavaScript_is_on_or_off Should I add 'docs required' so this doesn't get closed without alteration to that page?
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your contributions!

              _main:
              @ BB#0:
                      push    {r7, lr}
                      mov     r7, sp
                      sub     sp, #4
                      movw    r0, :lower16:(L_.str-(LPC0_0+4))
                      movt    r0, :upper16:(L_.str-(LPC0_0+4))
              LPC0_0:
                      add     r0, pc
                      bl      _printf
                      movs    r1, #0
                      movt    r1, #0
                      str     r0, [sp]                @ 4-byte Spill
                      mov     r0, r1
                      add     sp, #4
                      pop     {r7, pc}
               
                      .section        __TEXT,__cstring,cstring_literals
              L_.str:                                 @ @.str
                      .asciz   "This code is now upstream!"
              

              Show
              poltawski Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4-byte Spill mov r0, r1 add sp, #4 pop {r7, pc}   .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

                People

                • Votes:
                  1 Vote for this issue
                  Watchers:
                  7 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    8/Jul/13