-
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
Selection buttons #121
Selection buttons #121
Changes from 7 commits
e8c5341
f2c4765
db96cb6
a9f16a0
65eaa76
c266144
58f6d90
3b219a6
3236668
4d356b0
2ce29ba
b4d22f4
beb1cf7
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,158 @@ | ||
(function () { | ||
"use strict" | ||
var root = this, | ||
$ = root.jQuery; | ||
|
||
if (typeof GOVUK === 'undefined') { root.GOVUK = {}; } | ||
|
||
var BaseButtons = function ($elms, opts) { | ||
var _this = this; | ||
|
||
this.$elms = $elms; | ||
this.selectedClass = 'selected'; | ||
this.focusedClass = 'focused'; | ||
if (opts !== undefined) { | ||
$.each(opts, function (optionName, optionObj) { | ||
_this[optionName] = opts[optionName]; | ||
}); | ||
} | ||
this.setup(); | ||
this.bindEvents(); | ||
}; | ||
BaseButtons.prototype.markFocused = function ($elm, state) { | ||
var elmId = $elm.attr('id'); | ||
|
||
if (state === 'focused') { | ||
$elm.parent('label').addClass(this.focusedClass); | ||
} else { | ||
$elm.parent('label').removeClass(this.focusedClass); | ||
} | ||
}; | ||
|
||
var RadioButtons = function ($elms, opts) { | ||
BaseButtons.apply(this, arguments); | ||
}; | ||
RadioButtons.prototype.setup = function () { | ||
var _this = this; | ||
|
||
this.selections = {}; | ||
$.each(this.$elms, function (index, elm) { | ||
var $elm = $(elm), | ||
radioName = $elm.attr('name'); | ||
|
||
if (typeof _this.selections[radioName] === 'undefined') { | ||
_this.selections[radioName] = false; | ||
} | ||
if ($elm.is(':checked')) { | ||
_this.markSelected($elm); | ||
_this.selections[radioName] = $elm.attr('id'); | ||
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. Rather than relying on the input to have an id could we just store a reference to 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. Yeah, I like that, will change accordingly. |
||
} | ||
}); | ||
}; | ||
RadioButtons.prototype.bindEvents = function () { | ||
var _this = this; | ||
|
||
this.$elms | ||
// some browsers fire the 'click' when the selected radio changes by keyboard | ||
.on('click change', function (e) { | ||
var $elm = $(e.target); | ||
|
||
if ($elm.is(':checked')) { | ||
_this.markSelected($elm); | ||
} | ||
}) | ||
.on('focus blur', function (e) { | ||
var state = (e.type === 'focus') ? 'focused' : 'blurred'; | ||
|
||
_this.markFocused($(e.target), state); | ||
}); | ||
}; | ||
RadioButtons.prototype.markSelected = function ($elm) { | ||
var radioName = $elm.attr('name'), | ||
previousSelection = this.selections[radioName]; | ||
|
||
if (previousSelection) { | ||
$('#' + previousSelection).parent('label').removeClass(this.selectedClass); | ||
} | ||
$elm.parent('label').addClass(this.selectedClass); | ||
this.selections[radioName] = $elm.attr('id'); | ||
}; | ||
RadioButtons.prototype.markFocused = function ($elm) { | ||
BaseButtons.prototype.markFocused.apply(this, arguments); | ||
}; | ||
|
||
var CheckboxButtons = function ($elms, opts) { | ||
BaseButtons.apply(this, arguments); | ||
}; | ||
CheckboxButtons.prototype.setup = function () { | ||
var _this = this; | ||
|
||
this.$elms.each(function (idx, elm) { | ||
var $elm = $(elm); | ||
|
||
if ($elm.is(':checked')) { | ||
_this.markSelected($elm); | ||
} | ||
}); | ||
}; | ||
CheckboxButtons.prototype.bindEvents = function () { | ||
var _this = this; | ||
|
||
this.$elms | ||
.on('click', function (e) { | ||
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. Does also listening for a 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. I've not tested the effects of change events on checkboxes as thoroughly as for radios across browsers. What about having the event names stored as properties on the instance so they could be relative to each type of object? For example |
||
_this.markSelected($(e.target)); | ||
}) | ||
.on('focus blur', function (e) { | ||
var state = (e.type === 'focus') ? 'focused' : 'blurred'; | ||
|
||
_this.markFocused($(e.target), state); | ||
}); | ||
}; | ||
CheckboxButtons.prototype.markSelected = function ($elm) { | ||
if ($elm.is(':checked')) { | ||
$elm.parent('label').addClass(this.selectedClass); | ||
} else { | ||
$elm.parent('label').removeClass(this.selectedClass); | ||
} | ||
}; | ||
CheckboxButtons.prototype.markFocused = function ($elm) { | ||
BaseButtons.prototype.markFocused.apply(this, arguments); | ||
}; | ||
|
||
root.GOVUK.RadioButtons = RadioButtons; | ||
root.GOVUK.CheckboxButtons = CheckboxButtons; | ||
|
||
var selectionButtons = function ($elms, opts) { | ||
var addToSet, | ||
$radios, | ||
$checkboxes; | ||
|
||
addToSet = function ($elm, $set) { | ||
if ($set) { | ||
$set = $set.add($elm); | ||
} else { | ||
$set = $elm; | ||
} | ||
return $set; | ||
}; | ||
|
||
$elms.each(function (idx, elm) { | ||
var $elm = $(elm); | ||
|
||
if ($elm.attr('type') === 'radio') { | ||
$radios = addToSet($elm, $radios); | ||
} else { | ||
$checkboxes = addToSet($elm, $checkboxes); | ||
} | ||
}); | ||
|
||
if ($radios) { | ||
new GOVUK.RadioButtons($radios, opts); | ||
} | ||
if ($checkboxes) { | ||
new GOVUK.CheckboxButtons($checkboxes, opts); | ||
} | ||
}; | ||
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. I think this method would be clearer as something more like: function selectionButtons($elms, opts) {
var $radios = $elms.filter("[type=radio]"),
$checkboxes = $elms.filter("[type=checkbox]");
if ($radios) {
new GOVUK.RadioButtons($radios, opts);
}
if ($checkboxes) {
new GOVUK.CheckboxButtons($checkboxes, opts);
}
} 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. Much, much better, thanks! |
||
|
||
root.GOVUK.selectionButtons = selectionButtons; | ||
}).call(this); |
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.
setup
doesn't feel like a very good name. What does this method actually do?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.
I tend to split any code that sets up the instance object into:
Explained that way I suppose it is ambiguous because it represents a container for every other action that needs to be performed around adding properties/methods to the instance.
In this context it's only really doing one thing, querying the DOM to find the radio in the set that's selected so maybe
getSelection
?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.
The separation is a good thing. Methods named generic things like setup often get used as a dumping ground so are best avoided.
I would possibly go with
getSelections
because it does it on every element. ButgetSelection
would be perfectly agreeable.