Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.3.4, 2.4.1
    • Fix Version/s: 2.5
    • Component/s: AJAX and JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      Run jshint (available from Node.js) (npm -g install jshint)over lib/yui/formchangechecker/formchangecheck.js for each branch

      In Firefox, Chrome and IE browsers:

      • Open the course settings page (form)
      • Refresh the page
        • Confirm that no warnings were shown
      • Enter some content
      • Refresh the page
        • Confirm that a warnings was shown before navigating away
      Show
      Run jshint (available from Node.js) (npm -g install jshint)over lib/yui/formchangechecker/formchangecheck.js for each branch In Firefox, Chrome and IE browsers: Open the course settings page (form) Refresh the page Confirm that no warnings were shown Enter some content Refresh the page Confirm that a warnings was shown before navigating away
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      Fix JS Hint with the form change checker

        Gliffy Diagrams

          Activity

          Hide
          Damyon Wiese added a comment -

          Hi Andrew,

          Thanks for submitting this the improvements make sense to me.

          I have one issue though - when declaring multiple variables with one var statement, the subsequent variables should be indented for readability.

          e.g.

          +                    var formid = 'form#' + this.get('formid'),
          +                    currentform = Y.one(formid);
          

          should be:

          +                    var formid = 'form#' + this.get('formid'),
          +                        currentform = Y.one(formid);
          

          Also - this is an improvement IMO and should not be backported.

          Show
          Damyon Wiese added a comment - Hi Andrew, Thanks for submitting this the improvements make sense to me. I have one issue though - when declaring multiple variables with one var statement, the subsequent variables should be indented for readability. e.g. + var formid = 'form#' + this.get('formid'), + currentform = Y.one(formid); should be: + var formid = 'form#' + this.get('formid'), + currentform = Y.one(formid); Also - this is an improvement IMO and should not be backported.
          Hide
          Damyon Wiese added a comment - - edited

          Note: There is more than one instance of that indenting issue - I only listed one.

          Show
          Damyon Wiese added a comment - - edited Note: There is more than one instance of that indenting issue - I only listed one.
          Hide
          Damyon Wiese added a comment -

          Just adding a subset of the browsers to the testing instructions as this is a low risk change.

          Show
          Damyon Wiese added a comment - Just adding a subset of the browsers to the testing instructions as this is a low risk change.
          Hide
          Andrew Nicols added a comment -

          Thanks Damyon,

          Adjusted as per your suggestions (indenting) and dropping the other branches.

          Submitting for IR.

          Show
          Andrew Nicols added a comment - Thanks Damyon, Adjusted as per your suggestions (indenting) and dropping the other branches. Submitting for IR.
          Hide
          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
          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
          Dan Poltawski added a comment -

          Integrated thanks Andrew.

          Show
          Dan Poltawski added a comment - Integrated thanks Andrew.
          Hide
          Andrew Nicols added a comment -

          Ankit, you may wish to see MDL-37626 for a suitable jshint configuration.

          Show
          Andrew Nicols added a comment - Ankit, you may wish to see MDL-37626 for a suitable jshint configuration.
          Hide
          Ankit Agarwal added a comment -

          Thanks Andrew, Just got jshint working after looking at details in a related issue.

          Show
          Ankit Agarwal added a comment - Thanks Andrew, Just got jshint working after looking at details in a related issue.
          Hide
          Ankit Agarwal added a comment -

          Andrew,
          You want a strict mode check right?
          I am getting following error with strict mode:-

          lib/yui/formchangechecker/formchangechecker.js: line 3, col 9, Missing "use strict" statement.
           
          1 error
          

          No errors with strict mode being false.

          Please suggest if tests needs to be failed.
          Thanks

          Show
          Ankit Agarwal added a comment - Andrew, You want a strict mode check right? I am getting following error with strict mode:- lib/yui/formchangechecker/formchangechecker.js: line 3, col 9, Missing "use strict" statement.   1 error No errors with strict mode being false. Please suggest if tests needs to be failed. Thanks
          Hide
          Andrew Nicols added a comment -

          Hi Ankit,

          No we're not enforcing strict at present, and I don't think that we're likely to either. Strict can be quite difficult to get right.
          To support it fully, we'd have to add a use strict to every function, and this could more breakages than it would solve.

          At the moment, I'm suggesting we use the jshintrc used by the YUI project as this will help us to get our modules to the same kind of standard as theirs.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Ankit, No we're not enforcing strict at present, and I don't think that we're likely to either. Strict can be quite difficult to get right. To support it fully, we'd have to add a use strict to every function, and this could more breakages than it would solve. At the moment, I'm suggesting we use the jshintrc used by the YUI project as this will help us to get our modules to the same kind of standard as theirs. Andrew
          Hide
          Ankit Agarwal added a comment -

          In that case, All is good.
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - In that case, All is good. Passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: