concerto / concerto-frontend

Frontend JS code for Concerto
http://concerto-signage.org
Other
7 stars 5 forks source link

fix html #48

Closed mfrederickson closed 9 years ago

mfrederickson commented 9 years ago

This fixes #47.

Instead of vulcanizing the entire screen.html, this only vulcanizes the polymer elements listed in the vulcanize_this.html file. It still inlines scripts and css but now it excludes the third party js files, which are stored under the gems vendor/assets directory (there is a grunt task to do this, see the README) and those js files are required in the gem's application.js file. vulcanize_this.html also excludes the wrapper html. Because the vulcanization task currently outputs html wrappers and head/body and a hidden div tags there is a manual step to tweak the output file so it can be used in the gem without causing problems with the markup (see the README). The output file is now written to the gem as an asset called concerto-frontend.html which is "imported" via the _frontend.html partial.

When testing this, you'll also need the front-fix branches from concerto, concerto-iframe, and concerto-remote-video repos. If you reject this PR, then you should reject the concerto/concerto-iframe#16 and concerto/concerto-remote-video#30 PRs also.

This still needs to be tested in a production environment. I'm working on that...

@gbprz I supposed if screen.html was only being used for the vulcanization, then we could remove that and rename the vulcanize_this.html to screen.html, and change the Gruntfile and README.

mfrederickson commented 9 years ago

I suppose we could use https://github.com/bomsy/grunt-regex-replace to automatically tweak the vulcanized file, if we decide to go with this PR.

mfrederickson commented 9 years ago

Tested fine in production for me.

gbprz commented 9 years ago

Looks great, thanks for the help! You're right about screen.html, it was only there for vulcanization so it can be removed or replaced with vulcanize_this, I don't mind either way.

Seems like we can move forward with merging this, the plugin changes, and the core Concerto updates unless you have anything else you want to add.

mfrederickson commented 9 years ago

Nope. Go for it. Thanks.