Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust creation of markup for Open Flash Chart #11951

Merged
merged 2 commits into from
May 25, 2018

Conversation

kerasai
Copy link
Contributor

@kerasai kerasai commented Apr 5, 2018

Overview

Certain JS libraries don't play nice with the <html>, <head>, and <body> tags within the string literals. For instance, New Relic's browser side monitoring automatically initializes itself based on the structure of the DOM, and with these "tags" found duplicated within the page, things don't end well.

Before

When using in environments where New Relic is utilized, the charts do not initialize. The following errors are thrown in the JS console:

Firefox: SyntaxError: unterminated string literal
Chrome: Invalid or unexpected token

After

The charts initialize as expected, no JS errors.

Technical Details

The only change is to break up the string literals that are getting parsed as tags, and concatenating the string together.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@seamuslee001
Copy link
Contributor

Jenkins add to whitelist

@colemanw
Copy link
Member

colemanw commented Apr 16, 2018

Hey @kerasai thanks for this patch. Makes sense to me, although it might be confusing to future devs who come along. Could I request a couple little tweaks:

  • Just put the strings in instead of declaring variables for them (e.g. document.write('<' + 'html' + '><' + 'head' + '>...)
  • Add a comment above explaining why this concatenation is necessary (and avoid putting the string <head> or <html> in the comment! 😆 )

@totten
Copy link
Member

totten commented May 25, 2018

Test failure is a false-negative. Merging. Thanks @kerasai.

@totten totten merged commit b48045a into civicrm:master May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants