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

Add document level events to selection buttons #139

Merged
merged 9 commits into from
Oct 17, 2014

Conversation

tombye
Copy link
Contributor

@tombye tombye commented Oct 13, 2014

The current version of GOVUK.selectionButtons (see #121) works by taking a jQuery-wrapped collection of DOM nodes as the first parameter, and binding events to them:

var $buttons = $("label input[type='radio'], label input[type='checkbox']");
GOVUK.selectionButtons($buttons);

This sometimes doesn't work on pages where the HTML is replaced by AJAX because the elements those events are bound to may no longer be in the page.
This pull request (PR) makes a new version called GOVUK.SelectionButtons which has the ability to take a selector string instead, which it uses to assign a live event to the document:

var selectionButtons = new GOVUK.SelectionButtons("label input[type='radio'], label input[type='checkbox']");

This means once you've called GOVUK.selectionButtons, any changes to the content of the page will not effect the functionality. This is backwards-compatible, the existing interface is still supported.

This PR also includes a new destroy method for clean-up of the events it creates:

selectionButtons.destroy();

In addition, the tests for selection-buttons have been rewritten. The previous ones were too tied to the implementation and didn't describe the requirements very well.

Includes the loss of GOVUK.RadioButtons and GOVUK.CheckboxButtons.
The old ones were too coupled to the implementation. These are written
to better reflect the behaviours expected.
Includes fix for misplaced comma.
this.focusEvents = 'focus blur';
this.setInitialState($(elms));
this.setEventTypes(elms);
this.bindEvents(elms);
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this abstraction where setEventTypes creates this.bindEvents quite confusing to read. If I follow it through correctly you could drop line 17 and SelectionButtons.prototype.setEventTypes and create something like:

SelectionButtons.prototype.bindEvents = function (elmsOrSelector) {
  if (typeof elmsOrSelector === 'string') {
    this.bindDocumentLevelEvents(elmsOrSelector);
  } else {
    this.bindElementLevelEvents(elmsOrSelector);
  }
}

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 like the use of elmsOrSelector to make it explicit what's happening and losing SelectionButtons.prototype.setEventTypes too. I ended up doing a similar thing with SelectionButtons.prototype.markSelected.

I would like to keep the local variables that identify the type we end up using (and include something that points to the fact the variable sent into SelectionButtons.prototype.bindElementLevelEvents is a jQuery object.

How about this?

SelectionButtons.prototype.bindEvents = function (elmsOrSelector) {
  var selector,
      $elms;

  if (typeof elmsOrSelector === 'string') {
    selector = elmsOrSelector;
    this.bindDocumentLevelEvents(selector);
  } else {
    $elms = $(elmsOrSelector);
    this.bindElementLevelEvents($elms);
  }
};

(I'll change the bind prefix separately btw, including it here so as not to confuse issues.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how much value there is in an explicit identification that the object sent to bindElementLevelEvents is a jQuery object. To me that is a concern of the method receiving not sending as if another developer wanted to find out what was being sent then I would look at the function definition of bindElementLevelEvents. However, having the extra variables doesn't hurt so it is up to you.

I wouldn't add the extra $elms = $(elmsOrSelector); over just $elms = elmsOrSelector; though as that adds an extra jQuery wrap which has no value but does incur computation.

@edds
Copy link
Contributor

edds commented Oct 13, 2014

I am finding the naming of the internal methods in this being named bindThing gets a little confusing now we are using the real Function.prototype.bind. I think they would be better named addThing which better matches addEventListener.

.off('focus blur', selector);
SelectionButtons.eventSelectors = $.grep(SelectionButtons.eventSelectors, function (entry) {
return entry !== selector;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method just for testing? Otherwise if it provides value it should be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was written just for testing. I can imagine it might add some value as a 'teardown' method if you're doing a lot with live events.

I'd be interested in your thoughts on leaving it in. Not been in this situation before.

@tombye
Copy link
Contributor Author

tombye commented Oct 13, 2014

For that last one, are you just talking about bindEvents?

@tombye
Copy link
Contributor Author

tombye commented Oct 13, 2014

Ignore that, it's clear what the others are. Agree that it would add clarity so changing all methods prefixed with bind.

Allows storage of returned instance & for the destroy method (previously
`GOVUK.selectionButtons.removeEventsFor`) to work on that instance.
Includes a note on dealing with deprecated functionality.
@tombye
Copy link
Contributor Author

tombye commented Oct 14, 2014

After a chat with @edds about GOVUK.selectionButtons being unnecessary and GOVUK.selectionButtons.removeEventsFor being better as an instance method I've changed the interface and some of the structure.

The added destroy method allows users to unbind any events GOVUK.SelectionButtons has set. This moves responsibility for all events set onto the user which is why all internal tracking of events has been removed.

if (typeof elmsOrSelector === 'string') {
$elms = $(elmsOrSelector);
this.selector = elmsOrSelector;
this.setInitialState($(this.selector));
Copy link
Contributor

Choose a reason for hiding this comment

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

this.setInitialState($(this.selector)); can be this.setInitialState($elms); as you have already created the jQuery object.

@edds
Copy link
Contributor

edds commented Oct 15, 2014

One minor comment. Otherwise this is looking good to me. We will need to make sure we bump the toolkit version by a major number for this and make the reasoning clear in the changelog.

@tombye
Copy link
Contributor Author

tombye commented Oct 15, 2014

Yes, agreed. If this is merged I can do the version bump with a full explanation.

dsingleton added a commit that referenced this pull request Oct 17, 2014
…lection-buttons

Add document level events to selection buttons
@dsingleton dsingleton merged commit bc2b6f1 into master Oct 17, 2014
@dsingleton dsingleton deleted the add-document-level-events-to-selection-buttons branch October 17, 2014 13:34
gemmaleigh pushed a commit to alphagov/govuk_elements that referenced this pull request Nov 19, 2014
# 2.0.1

- Fix new grid helpers to ensure content using %site-width-container is
centred in IE.

# 2.0.0

- Add support for selection-button events to be document-level
alphagov/govuk_frontend_toolkit#139

- The `GOVUK.selectionButtons` interface is now deprecated. Details are
in the
[README](https://github.com/alphagov/govuk_frontend_toolkit#deprecated-f
unctionality).
dsingleton pushed a commit to alphagov/static that referenced this pull request Jan 26, 2015
Some GOV.UK pages, like Smart Answers, load content via JS, including
radio buttons or checkboxes. These will be added to the DOM after the
selector is run and the resulting elements are passed to SelectionButtons

SelectionButtons can also take a string selector and use document
events (alphagov/govuk_frontend_toolkit#139)
so use that and any JS loaded content  will get the behaviour too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants