This is a more declarative way to check if the context is a jQuery el…

[t3dodson]

Please consider the following before submitting a pull request:

Guidelines for contributing: https://github.com/chartjs/Chart.js/blob/master/CONTRIBUTING.md

Example of changes on an interactive website such as the following:

• http://jsbin.com/
• http://jsfiddle.net/
• http://codepen.io/pen/
• Premade template: http://codepen.io/pen?template=JXVYzq
  …ement.

Also no need to check .getContext twice.

[etimberg]

Will this continue to work even if jQuery is not in the global namespace? I think that case could happen in a commonJS style build.

I also think we should, if possible, add a test for this

[simonbrunel]

I'm not sure we should make a special case for jQuery. Actually, I already changed this lookup in the cleanup I'm doing around the canvas but also added support for a string id. Will be similar to this:

var Chart = function(element, config) {

    // ...

    if (typeof(element) === 'string') {
        element = document.getElementById(element);
    }
    if (element.length) {
        element = element[0];
    }
    if (element instanceof HTMLCanvasElement) {
        element = element.getContext('2d');
    }
    if (!element instanceof CanvasRenderingContext2D) {
        // ERROR
        return;
    }

    context = element;

    // ...

This is in anticipation of the new way to create a chart via a div element.

What do you think?

[t3dodson]

@simonbrunel This updated code is clear. However I still think a check for jquery could be used. Checking for a length property can work for a lot of edge cases.

@etimberg This should work even if jquery isn't defined globally. This is a short-circuiting operation. typeof(asdfjasdfjasdfjasdf) doesn't throw an exception it returns the string 'undefined'. I think the if statement would only be satisfied if jquery is defined globally, and the context is an instance of jquery.

[t3dodson]

@etimberg I can add an explicit test case if you think this would be acceptable.

[simonbrunel]

That's exactly the point: handle more cases and not target jQuery specifically. If it's an array (or compatible object), simply deals with the first element. Saves some code and a special case.

[etimberg]

I agree with @simonbrunel that if we can have a more generic check (for a bunch of different cases) then we should do that. I would definitely like a test either way though

[simonbrunel]

I already have these changes locally, with unit tests but still need some work until being able to submit a PR (since it's part of a more important refactor).

[etimberg]

Closing, this functionality was added in #3474
Thank you for opening this PR.