Uploaded image for project: 'Plugins'
  1. Plugins
  2. CONTRIB-1778

Code review comments

    XMLWordPrintable

Details

    • Bug
    • Status: Resolved
    • Minor
    • Resolution: Fixed
    • 1.9.7
    • None
    • None
    • MOODLE_19_STABLE

    Description

      Looks like really good work.

      Some minor comments from looking at the code:

      • There are much better ways to get data from PHP to JS than the one you use in question.html. Try

        <?php
        $flashvars = array(
            'direction' => $question->options->direction,
            'quality' => $question->options->quality,
            // etc.
            'responseFunc' => 'setResponse' . $nameprefix,
        );
        ?>
        <script type="text/javascript">
            var flashvars = <?php echo json_encode($flashvars); ?>

      • Why does inMoodle need to be a function, not a flashvar? and is getState actually used?
      • Would be nice to spell out the interval types Major, Minor, etc. with lang strings on the settings form.
      • Did you try using the right unicode character for flat, etc. in that settings dropdown?
      • Should you have a readme file in the swfobject folder to acknowledge that it is third-party code?
      • question/type/interval/lang/en_utf8/help/interval/interval.html should use <h1> for the title.
      • Moodle coding guideline is 4 spaces for indent, not tabs.
      • I normally double-indent the following lines when wrapping a line of code onto multiple lines.

      As a cool idea for the future. It would be nice for the teacher if you could use the same flash applet on the edit form, for the teacher to enter the starting note. You probably already thought of that.

      Hope that is helpful.

      Tim. Quiz module / Question bank maintainer.

      Attachments

        Issue Links

          Activity

            People

              brissone Eric Brisson
              timhunt Tim Hunt
              Eric Brisson
              Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: