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

          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