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

Allow tracking of all trackable links within a container #883

Merged
merged 3 commits into from Jan 25, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Jan 20, 2017

Creating a single event listener is more efficient than one for every link that could be clicked. It also reduces the number of unique modules that must be started.

  • Use new pattern on breadcrumbs and related links
  • I've tested the correct events are generated in frontend

Will make https://trello.com/c/62Gy8pyj/77-track-interactions-with-contents-lists-1-s more efficient.

cc @tijmenb @alecgibson @nickcolley

fofr added 3 commits Jan 20, 2017
* Remove specifics about related links and breadcrumbs, instead test
the feature – sending clicks with custom dimensions set
Creating a single event listener is more efficient than one for every
link that could be clicked. It also reduces the number of unique
modules that must be started.
Create fewer modules and event listeners on breadcrumbs and related
links
@fofr fofr closed this Jan 20, 2017
@fofr fofr reopened this Jan 20, 2017
if (element.is(trackable)) {
element.on('click', trackClick);
} else {
element.on('click', trackable, trackClick);
}

This comment has been minimized.

@alecgibson

alecgibson Jan 20, 2017
Contributor

Wouldn't it be easier to register a listener on all "trackable" descendants?
element.find(trackable).click(trackClick); (You'd then be able to drop the slightly weird parents check, in trackClick).

This comment has been minimized.

@fofr

fofr Jan 20, 2017
Author Contributor

That would create more event listeners and be less efficient.

This comment has been minimized.

@alecgibson

alecgibson Jan 20, 2017
Contributor

Is the performance difference actually noticeable? I'm not convinced that it's worth obscuring the code like this.

This comment has been minimized.

@nickcolley

nickcolley Jan 20, 2017
Contributor

I'm not sure how sophisticated jQuery's event delegation is however if it works as I think it does then using .find will add a listener for each element, rather than one listener.

There might be a way we can delegate all links however which would make the code simpler to understand and be performant.

This comment has been minimized.

@alecgibson

alecgibson Jan 20, 2017
Contributor

Oh I think it will almost certainly add a listener to each node. My point is: does it matter? This feels a little like premature optimisation. I do like the simplicity of a single module instantiation, but I think optimising away a handful of extra event listeners on a page is surely overkill?

This comment has been minimized.

@alecgibson

alecgibson Jan 20, 2017
Contributor

Yeah, I'm fine with it in principle, I just think it's making this particular bit of code less readable. Of course fine to leave - just my two cents.

This comment has been minimized.

@fofr

fofr Jan 20, 2017
Author Contributor

Here's a perf test for the various patterns:
http://jsperf.com/click-perf

This comment has been minimized.

@fofr

fofr Jan 20, 2017
Author Contributor

I could move the logic into a named function so it's clear what's happening?

This comment has been minimized.

@nickcolley

nickcolley Jan 20, 2017
Contributor

I'd agree if this wasn't generic, but since this could be used anywhere and with any amount of links, I'd rather us just do this now since memory leaks are hard to debug and crop up at the worst times.

This comment has been minimized.

@alecgibson

alecgibson Jan 20, 2017
Contributor

How about scrapping this if statement (ie call the handler on any click inside this element). Then in the handler, you could simply say var $el = $(evt.target).closest(trackable) and return early if no trackable element was found?

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Jan 24, 2017

I've tested this branch alongside government-frontend and it's working as expected, do we need to update some docs to indicate the new usage?

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 24, 2017

I don't think there are docs at the moment.

@alecgibson
Copy link
Contributor

@alecgibson alecgibson commented Jan 25, 2017

@fofr How well does this cope with nesting? What happens in these cases?

  1. Two tracked items are nested within one another (eg this possibly happens in the service-manual-frontend accordion?)
  2. Two tracking modules are nested inside each other. For example, let's say so many elements are tracked on a page that someone decides to move the tracking module up to some wrapper. However, that wrapper might contain, eg breadcrumbs, which also instantiate the module. Would we get a double tracking event fired?
@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 25, 2017

@alecgibson The modules shouldn't be nested.

  1. The most nested item would be tracked
  2. Click events would be tracked twice

Do you have an example of either of these happening intentionally?

If more advanced tracking of multiple features with a single click was a required feature we'd develop for it when it was needed.

@alecgibson
Copy link
Contributor

@alecgibson alecgibson commented Jan 25, 2017

@fofr My point is more that it's easy to accidentally nest modules (for example, you might forget that breadcrumbs already instantiate the module). Given that the whole point of this module is to accurately track how many times things were clicked on, I think this is important to guard against.

As for complex element tracking, see https://github.com/alphagov/service-manual-frontend/blob/master/app/assets/javascripts/modules/accordion-with-descriptions.js#L284

These are both nasty side-effects of this event delegation pattern (if listeners are attached to all the DOM nodes instead, then an event is fired every time an event propagates through them, as expected).

If this is a pattern that we intend to stick to, I'd suggest creating some sort of well-tested JavaScript Event Delegator object, who takes a callback and a selector, and makes sure that things like nesting are gracefully handled. This sort of refactoring could certainly be left for the next time we implement this pattern, but I definitely think we should be guarding against weird nesting behaviour, because it's highly unintuitive and could be hard to track down.

@fofr
Copy link
Contributor Author

@fofr fofr commented Jan 25, 2017

it's easy to accidentally nest modules (for example, you might forget that breadcrumbs already instantiate the module).

If we see this become a problem we can fix it then, and build an advanced event delegator when we need to. I agree there is some risk, but also that the risk is small and similar to misuse of the module pattern we already have.

We have a similar pattern in place on admin tools and haven't had these issues:
https://github.com/alphagov/govuk_admin_template/blob/master/app/assets/javascripts/govuk-admin-template/modules/track_click.js

@alecgibson
Copy link
Contributor

@alecgibson alecgibson commented Jan 25, 2017

@fofr sounds okay to me - the thought just occurred to me and I thought I'd flag it. If you think it's an acceptable risk, then that's fine.

@nickcolley
Copy link
Contributor

@nickcolley nickcolley commented Jan 25, 2017

Thanks @fofr and @alecgibson 👍 😄

@nickcolley nickcolley merged commit 73c5f24 into master Jan 25, 2017
1 check passed
1 check passed
default "Build #1443 succeeded on Jenkins"
Details
@nickcolley nickcolley deleted the track-clicks-on-container branch Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.