Feature/initial module #2
Conversation
| scriptTag.type = "text/javascript"; | ||
| scriptTag.async = true; | ||
| scriptTag.src = "components/skrollr/dist/skrollr.min.js"; | ||
|
|
There was a problem hiding this comment.
Why inject the skrollr library tag using javascript? This presumes the person using this directive has the same directory structure as us e.g. has the skrollr lib in app/components/. Also this won't work, even for us, when all the javascript is concatenated and/or minified.
There was a problem hiding this comment.
Yeh I was concerned about this too. The reason I have used this method is we need to initialise skrollr and directives after it has loaded.
Just including the script in the skrollr is never defined before it is used.
There was a problem hiding this comment.
https://github.com/Prinzhorn/skrollr#lets-get-serious
You just need to wait till the document loads before initialising your directives. You can use the window.onload event to be sure skrollr is defined. It will only run once all assets e.g. css files, javascript files have been loaded.
There was a problem hiding this comment.
Ahh for some reason I had assumed angular ran within window.load anyway so neglected to think of it. Does the trick, cheers.
|
I've refactored the skrollr initialisation and added an snSkrollrInit directive which allows one to pass in init options like this: <body sn-skrollr-init="{ smoothScrolling: true }">
...
</body> |
|
Instead of using a directive for this why not use a provider? For example in routeProvider you configure the ngRoute service before the app is run. Providers are perfect for reusable services that require a one time config before the app is run. |
|
I have now replaced the initialisation directive and service with a provider which enables skrollr to be configured in the angular config phase and initialised within |
|
|
||
| var _this = this; | ||
|
|
||
| this.init = {}; |
There was a problem hiding this comment.
would it make more sense to call this object config instead of init?
|
We probably don't need to use the promise library anymore either. |
|
looks good, i think you just need to fix the build and it will be ready for a release. Also you may want to start think about writing test to improve code coverage, it should be fairly straight forward though, since this is very small. (actually ignore that, i meant tests for the thisissoon-frontend) |
|
Looks good 👍 coveralls seems to be slow, will merge and release once it fixes itself |
This PR adds the SkrollrService and snSkorllr directive which load and then initialise the skrollr animation elements. The directive refreshes skrollr when an element with the directive is loaded.