-
Notifications
You must be signed in to change notification settings - Fork 107
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.
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
Feature/anchor buttons #297
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
(function(global, settings) { | ||
"use strict"; | ||
|
||
var $ = global.jQuery; | ||
var GOVUK = global.GOVUK || {}; | ||
|
||
GOVUK.anchorButtons = { | ||
|
||
// default configuration that can be overridden by passing object as second parameter to module | ||
config: $.extend({ | ||
// setting to true will disable the init function upon execution | ||
disableInit: false, | ||
// the target element(s) to attach the shim event to | ||
selector: 'a[role="button"]', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add this role to other elements such as spans? Maybe the selector should be just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// array of keys to match against upon the keyup event | ||
keycodes: [ | ||
32 // spacekey | ||
], | ||
}, settings), | ||
|
||
// event behaviour (not a typical anonymous function for resuse if needed) | ||
triggerClickOnTarget: function triggerClickOnTarget(event) { | ||
var code = event.charCode || event.keyCode; | ||
// if the keyCode/charCode from this event is in the keycodes array then | ||
if ($.inArray(code, this.config.keycodes) !== -1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nit (sorry) but I'm wondering, if we're only ever going to be shimming the spacekey, could we just do a boring code === 32 here instead of an array comparison, not super important (sorry again :P) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again fair point. Originally there was talk of triggering on enter/return as well. Left it open to additional key-codes just in case more than anything. Happy to change if we feel that makes it more readable/explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally for me I prefer to avoid anticipating future updates since more often than not they don't happen but don't think this blocks merging, so up to you. ✌️ |
||
event.preventDefault(); | ||
// trigger the target's click event | ||
$(event.target).trigger("click"); | ||
} | ||
}, | ||
|
||
// init function (so it can be executed again if needed) | ||
init: function init() { | ||
var $elms = $(this.config.selector); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we attach a listener to the document rather than one for each button? $(document).on('keyup', this.config.selector, this.triggerClickOnTarget.bind(this)); My thought is that this way we only have one delegated listener rather than potentially a few? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, good point I think that's probably better. 👍 Will do that. |
||
// if found elements that match the selector in config (or settings) then | ||
if($elms.length > 0) { | ||
// iterate them giving access to current scope | ||
$elms.each(function(index, elm){ | ||
// attach triggerClickOnTarget with current scope to the keyup event | ||
$(elm).on('keyup', this.triggerClickOnTarget.bind(this)); | ||
}.bind(this)); | ||
} | ||
} | ||
|
||
}; | ||
|
||
// if disbaleInit is not true then run the init method | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good spot! Will fix. |
||
if(!GOVUK.anchorButtons.config.disableInit) { | ||
GOVUK.anchorButtons.init(); | ||
} | ||
|
||
// hand back to global | ||
global.GOVUK = GOVUK; | ||
|
||
})(window); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing EOF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment block at the top explaining what the file does? E.g. https://github.com/alphagov/govuk_frontend_toolkit/blob/master/javascripts/govuk/stop-scrolling-at-footer.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.. I'll get that done and reply back when I've updated this PR.