I just noticed that the Highcharts Javascript doesn't pass context to the jQuery constructor. This will generally only have a small impact, and you already work around potential problems by using .once(), but I still think best practice would be to include it. It will also help avoid problems in some edge cases, and shouldn't in any way negatively affect normal usage.

CommentFileSizeAuthor
#3 charts-include-context-in-2679391-3.patch1.65 KBNWOM
#2 2679391-2--jQuery_context_parameter.patch650 bytesdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Vizala commented
Status: Active » Needs review
FileSize
2679391-2--jQuery_context_parameter.patch650 bytes

(Trivial) Patch attached, would be great if someone could test/review to verify everything still works as expected.

NWOM’s picture

NWOM CreditAttribution: NWOM commented
FileSize
charts-include-context-in-2679391-3.patch1.65 KB

Here is a patch that includes the context for Google Charts as well. Please review

  • andileco committed b1b89a7 on 7.x-2.x authored by NWOM 
    Issue #2679391 by drunken monkey, NWOM: Include context in jQuery...
andileco’s picture

andileco CreditAttribution: andileco at JSI Research & Training Institute, Inc. (JSI) commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.