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

Move Report Chart Generation to a Separate Script #7022

merged 8 commits into from Nov 21, 2018


Copy link

@spencerfinnell spencerfinnell commented Nov 7, 2018

Properly enqueuing ChartJS means the position it is loaded cannot be guaranteed. If it is used by another script that outputs in the footer it will be moved to the footer as well. This means the inline Javascript printed in the page to render charts will be run immediately and without the proper dependency (charts are broken in release/3.0 currently).

This PR fixes that by building charts in a JS file that is dependent on the main reporting javascript, which can be properly enqueued.

I need a bit of input on some potential abstraction that could be done to facilitate the extra data that was added to the build_config() method in the chart manifest class. @DrewAPicture I think that's you?

To test:

  1. npm run dev (I can commit the built scripts if needed)
  2. View the reports and use the charts.

To do:

Copy link

@spencerfinnell Thanks for taking the lead on this. The JavaScript improvements are amazing and frankly above my head! So I appreciate you diving in.

Copy link

@spencerfinnell still working on this or ready for testing?

Copy link
Contributor Author

Good to test! The compiled JS is included in the branch so you shouldn't need to run anything.

@pippinsplugins pippinsplugins merged commit ee7cddc into release/3.0 Nov 21, 2018
@pippinsplugins pippinsplugins deleted the js/report-charts branch November 21, 2018 18:53
Copy link

JJJ commented Nov 21, 2018

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

4 participants