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.

Sign up for GitHub

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

Jump to bottom

script is inserted before html #2

Closed
zam6ak opened this issue Nov 21, 2016 · 6 comments
Closed

script is inserted before html #2

zam6ak opened this issue Nov 21, 2016 · 6 comments

Comments

@zam6ak
Copy link

zam6ak commented Nov 21, 2016

Is there a reason why the script that is automatically injected cannot be inserted via script tag?

Currently it is being inserted before document even begins, throwing off validation, and in some cases causing layout issue. Most importantly, the script injected contains report data, which in my case is rather large, so rendering is slowed down (compared to plain html recipie for example)

A simple solution would be just to require including a script link in either head or at the bottom.
It would be template author's responsibility to do so.

Another option would be to use new asset helper with special name.
@pofider
Copy link
Contributor

pofider commented Nov 22, 2016

  1. It is in the beginning just because of my laziness. I should find the <head> in html and put it just after it if is there. Lets do this.

  2. I understand that big data are problem and not always required. Lets add a flag to the template which excludes the data from the script if activated.

  3. The jsreport browser client sdk script should be linked to speed up the html page load

I would apply these changes if you agree?

@zam6ak
Copy link
Author

zam6ak commented Nov 22, 2016

  1. does it have to be in the head? Could it be after the page loads towards bottom (like what Bootstrap example template looks like)? On one hand, automatically inserting it is kind of neat...on the other hand, I still think being able to link to this .js via asset would give a lot more flexibility....Is this script the only difference between 'html' and 'html-with-browser-client' recipes? If it is, I would, if may be so bold, recommend:
  • a) make the script a special asset (that would either include or exclude data). this would would allow authors to add it anywhere in template (or at lease anywhere in the head if it must be loaded early)
  • b) you could then deprecate 'html-with-browser-client' (just use 'html' and add this script if needed)

Lastly, if it must be in the <head>, add it right before closing </head> tag...

  1. Yes, a flag to exclude data would be neat

  2. Definitely yes (again, another case for making it a special asset)

@pofider
Copy link
Contributor

pofider commented Nov 22, 2016

Ok, it doesn't need to be at the beginning, that is true.

The recipe actually also adds to the page the current template, options and input data. This what you can't do with asset. So I think it makes sense to keep the recipe.

However you are free to simply link the jsreport sdk and use html recipe, if you don't need the above
<script src="/extension/browser-client/public/js/jsreport.min.js"></script>

@zam6ak
Copy link
Author

zam6ak commented Nov 22, 2016

ok I didn't know about other stuff...
Will the link work with appPath? Meaning with Ngnix proxying?

@pofider
Copy link
Contributor

pofider commented Nov 22, 2016

Well, no...

You would have to add the path manually
<script src="/reporting/extension/browser-client/public/js/jsreport.min.js"></script>

but then the script wan't now the serverUrl anyway. It is also part of the recipe's job. So probably better to use it and the omitting data switch will solve the perf problem.

pofider added a commit that referenced this issue Nov 23, 2016
@pofider
reference jsreport sdk as link at the end of the document and introdu…
66d3a15 
…ce omitDataFromOutput #2
@pofider
Copy link
Contributor

pofider commented Nov 23, 2016

Done in 1.1.0

@pofider pofider closed this as completed Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@zam6ak @pofider