Skip to content
This repository was archived by the owner on Jan 24, 2018. It is now read-only.

Support AMD #41

Open
ethanresnick wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support AMD #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
40941bf
Support AMD
ethanresnick Apr 20, 2014
f1c6f9b
Merge branch 'master' into amd
ethanresnick Apr 20, 2014
11d2928
Add bower.json
ethanresnick Nov 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ module.exports = function(grunt) {
globals: {
window: true,
document: true,
navigator: true
navigator: true,
define: true
}
},
all: ['src/**/*.js']
Expand Down
17 changes: 17 additions & 0 deletions bower.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"name": "skrollr-menu-amd",
"version": "0.1.12",
"homepage": "https://github.com/ethanresnick/skrollr-menu",
"authors": [
"Ethan Resnick <ethan.resnick@gmail.com>"
],
"main": "dist/skrollr.menu.min.js",
"license": "MIT",
"ignore": [
"**/.*",
"node_modules",
"bower_components",
"test",
"tests"
]
}
4 changes: 2 additions & 2 deletions dist/skrollr.menu.min.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 21 additions & 10 deletions src/skrollr.menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,25 @@
*
* Free to use under terms of MIT license
*/
(function(document, window) {
(function (window, document, skrollrMenuFactory) {
'use strict';

//In case the page was opened with a hash, prevent jumping to it.
//This code runs immediately, before skrollr may have loaded (if AMD's in use).
//http://stackoverflow.com/questions/3659072/jquery-disable-anchor-jump-when-loading-a-page
window.setTimeout(function() {
Copy link
Copy Markdown
Owner

@Prinzhorn Prinzhorn Apr 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this code executed once I do require(['skrollr-menu'], function() {}) (which means too late)?

By the way, where is the name of this module specified? I know nothing about AMD, but looking at Prinzhorn/skrollr#409 I can see that it's defined as skrollr. I mean, how do you required the menu plugin?

Copy link
Copy Markdown
Author

@ethanresnick ethanresnick Apr 27, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's taken me a bit to get back to this.

When this code runs will depend on the require configuration, but there's no reason to assume it will generally run later than the non-AMD setup we have now. In the non-AMD version, it runs only after skrollr has been downloaded and executed (since it must be included before skrollr-menu), and then only after skrollr-menu's been downloaded. In the AMD version, the code will run after the AMD loader's been downloaded and executed, and then after the skrollr-menu module's been downloaded. But it won't have to wait for the skrollr module to donwload (AMD loaders download the resources in parallel) or for the skrollr-menu module to start executing (since this code is defined outside the module body).

Perhaps the bigger issue, though, is that the AMD loader won't block the page's rendering. So even if the browser runs the code at about the same time in either case, it may have already shown the rendered page to the user under the AMD version, causing a visible jump as the code scrolls back to the top. The only way to prevent this, I think, would be to put this code in the HTML directly (making sure it's blocking but not taking an extra request) rather than use AMD. A bit annoying, but probably the best solution.

Re the module's name: it actually doesn't need one, and not naming it arguably makes it a bit more flexible. Only the skrollr module needs a hardcoded name, so that the plugins can dependably require it. With the setup in this commit, someone would require skrollr-menu like this:

require.config({
  //many config options go here
  //...
  paths: {
    "myNameForSkrollrMenu": "path/to/skrollr/menu"
  }
});
define(["myNameForSkrollrMenu"], function(skrollrMenu) {
   //my module that depends on skrollr-menu
}

Still, we can give it a name if you want.

if(window.location.hash) {
window.scrollTo(0, 0);
}
}, 1);

if (typeof define === 'function' && define.amd) {
define(['skrollr'], skrollrMenuFactory);
}
else {
skrollrMenuFactory(window.skrollr);
}
}(window, document, function (skrollr) {
'use strict';

var DEFAULT_DURATION = 500;
Expand All @@ -16,7 +34,6 @@
var MENU_TOP_ATTR = 'data-menu-top';
var MENU_OFFSET_ATTR = 'data-menu-offset';

var skrollr = window.skrollr;
var history = window.history;
var supportsHistory = !!history.pushState;

Expand Down Expand Up @@ -193,11 +210,5 @@
var _handleLink;
var _scale;

//In case the page was opened with a hash, prevent jumping to it.
//http://stackoverflow.com/questions/3659072/jquery-disable-anchor-jump-when-loading-a-page
defer(function() {
if(window.location.hash) {
window.scrollTo(0, 0);
}
});
}(document, window));
return skrollr.menu;
}));