Feature/amd #409
Feature/amd #409
Conversation
| <script data-main="scripts/main" src="scripts/require.js"></script> | ||
|
|
||
| <!--[if lt IE 9]> | ||
| <script type="text/javascript" src="dist/skrollr.ie.min.js"></script> |
There was a problem hiding this comment.
I guess as long as the plugins aren't loadable, this line will crash in IE because of missing global.
Also I just noticed that this file is 404 for a long time now (since it's in its own repo)...this is not related to the amd pr, but I need to fix this as well.
There was a problem hiding this comment.
same problem (404) for tests
There was a problem hiding this comment.
How would one handle the IE part with amd? Conditional comments are not an option I guess.
There was a problem hiding this comment.
How about making skrollr detect IE and then have it dynamically load ie plugin if necessary?
There was a problem hiding this comment.
The point is that "detect IE" is so easy with conditional comments but can't be used inside of a script. We can't reliably detect IE. I don't want to use the user agent string and parse the version.
We'd need to document something like this http://stackoverflow.com/questions/11846747/using-conditional-comments-with-requirejs-to-load-ie7-8-only-jquery . But I'm not a big fan of this particular solution using a class on the document.
There was a problem hiding this comment.
how about using modernizr
I don't want any dependencies and I don't want users to be forced to use any other lib.
Alternatively, we keep conditional comments approach and modify ie plugin to do smth like this:
The <smth-sensible-here> part scares me. That's not an option.
There was a problem hiding this comment.
i hear you. how about this: keep conditional comments and make sure skrollr include happens after the ie plugin. let the plugin provide patches through the window object and then merge them gracefully into the skrollr, smth like this:
// inside skrollr.js
if(typeof window.skrollr !== 'undefined'){
// clone the patch and wire it to the skrollr instance
// cleanup global scope if needed (if using require)
}
There was a problem hiding this comment.
Hm, I like the fact that skrollr doesn't need to know anything about the IE plugin and I'd like to keep it that way.
How is this handled in the real world? I thought people would use amd all the time.
There was a problem hiding this comment.
I've been looking around for a more elegant solution but have not found anything at this point. As per your question (how it's usually handled), my guess is that most developers would use feature detection as opposed to conditional comments to address browser issues. Truth is IE is pain in the ass any way you look at it, and people willing to support it in their projects (that use require) probably already have strategies in place to handle it. Chances are these include using feature detection of some sort.
There was a problem hiding this comment.
Feature detection is great, it's just that the conditional comments also prevent the file from being fetched in the first place.
Anyway, I think we should try to and do feature detection. It's just opacity and hls/hlsa/rgba, shouldn't be too hard. I don't know if we should wait with pulling the amd support until the IE plugin works standalone though to prevent people from having the same problems we're discussing here. Unfortunately I don't think I can work on this at least the next week, probably longer.
|
Please remove the How are we gonna proceed from here? Are you done? If so, do you want to start with the plugins as well? I think https://github.com/Prinzhorn/skrollr-menu is the most important. I've open an issue for discussion Prinzhorn/skrollr-menu#31 |
| @@ -1641,4 +1641,14 @@ | |||
|
|
|||
| //Animation frame id returned by RequestAnimationFrame (or timeout when RAF is not supported). | |||
| var _animFrame; | |||
|
|
|||
| //Expose skrollr as either a global variable or a require.js module | |||
| if (typeof define === 'function' && define.amd) { | |||
There was a problem hiding this comment.
This one space should be removed if (typeof as in the rest of the project.
If you know how to tell jshint, feel free to add it to this PR as well.
|
skrolls.min.js is out as per callmephilip@42e936c. I am not sure why it is still showing in the list of files. If you pull from the feature/amd branch, you'll notice that dist folder is not there |
|
@Prinzhorn : do you want me to squash the commits or keep rolling with the current 10? |
yes please. |
|
Added a note in readme and rebased. Looks good to me |
|
Great. I will pull this in the next days. I want to wait if we find an answer to the IE/conditional comments issue. |
check to see if require.js in available and if so, expose skrollr as a module; else put it on the global scope add an amd example using require.js to load skrollr require.js is added in scripts + main configuration module (standard require.js jazz) add tests for skrollr loading test that skrollr behaves accordingly based on the environment. require.js script is bundled in the test folder tell grunt to run skrollr loading tests during the build remove unused divs + styling in loading tests use single-line comment in module definition section remove jshint inline command and use globals config section in Grunt file adjust if statement formatting update readme with a note on require.js remove dist
|
Thanks again and sorry for the delay. I've merged this thing manually. I've called it experimental support, because we still need to update the plugins. |
|
No problem. Glad I was able to help |
expose skrollr as a require.js module