Skip to content
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

Merged
merged 3 commits into from
Aug 10, 2016
Merged
Changes from 2 commits
Commits
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
55 changes: 55 additions & 0 deletions javascripts/govuk/anchor-buttons.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
(function(global, settings) {
Copy link
Contributor

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

Copy link
Contributor Author

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.

"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"]',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this role to other elements such as spans?

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role

Maybe the selector should be just [role="button"].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about that one. Do we always want space to trigger a click on something with role="button" ? @LJWatson @aduggin to advise perhaps?

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

@paulmsmith paulmsmith Jul 27, 2016

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

typo ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing EOF.