Uploaded image for project: 'CiviCRM'
  1. CiviCRM
  2. CRM-12887

Js tidy-up on Contribution dashboard

    Details

    • Type: Bug
    • Status: Done/Fixed
    • Priority: Trivial
    • Resolution: Fixed/Completed
    • Affects Version/s: 4.4.0
    • Fix Version/s: 4.4.0
    • Component/s: None
    • Labels:
      None

      Description

      Per

      I wrote up most of what's needed for moving js out of smarty as part of http://wiki.civicrm.org/confluence/display/CRMDOC/Javascript+Reference.

      To answer your questions and some other observations about that particular tpl (templates/CRM/Contribute/Page/DashBoard.tpl):

      • The 'async' param can be removed when changing it from false, since it defaults to true. The reason it gets set to false is to work around the issue of php developers who are unfamiliar with javascript! PHP is very linear - whenever you call a function you know that the next line of code will not execute until that function returns. Not so in js, an ajax call will not wait before executing the rest of your script (that's the A in ajax) and the proper way to wait for its results is to pass in a "success handler" type callback function. Setting async to false is a pretty crappy way of mimicking the linearness of php, forcing the entire browser to freeze up until the -jax call completes. So when fixing the async flag you have to examine the code and see what assumptions have been made. I think there are 2 categories:
      o async has been set to false on purpose because the developer didn't know about callback functions and needed a way to wait for the ajax call to complete before executing the next bit of code.
      o async has been set to false accidentally - the async code was copy/pasted from elsewhere and it's not really needed here. I think this tpl falls into the latter category, since there is a success callback in use.

      • This script has 2 global functions: getChart and buildTabularView. These should be moved within the document.ready closure to get them out of the global scope. Whenever you get rid of globals you also have to do some grepping to make sure that there isn't any other code (especially onclick handlers set in the buildForm fn) that is trying to call those globaly. The "should do" is to move click handlers out of the html markup and instead use js to bind them the way it's being done within the document.ready closure in this tpl.

      • Looks like both of the $.ajax calls could be made into one-liners using $().load

      • Using CRM.url() you can do a cleaner job of generating those ajax urls, without resorting to concatenating extra stuff onto the end of an already formed url string.

      • I would change the closure to a simple cj(function($)... per the example in the wiki, and then you can replace cj with $ within the closure. The 'use strict' directive is important, as is linting with jsHint - don't skip that step!

        Attachments

          Activity

            People

            • Assignee:
              colemanw Coleman Watts
              Reporter:
              eileen Eileen McNaughton
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: