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
merged 8 commits into from Nov 21, 2018

Conversation

Projects
None yet
4 participants
@spencerfinnell
Copy link
Member

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:

  • Pie charts aren't working. See #7023

@spencerfinnell spencerfinnell requested a review from DrewAPicture Nov 7, 2018

@spencerfinnell spencerfinnell requested review from JJJ and sunnyratilal Nov 7, 2018

@DrewAPicture

This comment has been minimized.

Copy link
Member

DrewAPicture commented Nov 8, 2018

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

@pippinsplugins

This comment has been minimized.

Copy link
Member

pippinsplugins commented Nov 19, 2018

@spencerfinnell still working on this or ready for testing?

@spencerfinnell

This comment has been minimized.

Copy link
Member Author

spencerfinnell commented Nov 19, 2018

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

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@pippinsplugins pippinsplugins deleted the js/report-charts branch Nov 21, 2018

@JJJ

This comment has been minimized.

Copy link
Collaborator

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