Implement event delegation; refactor JS and tests #1836

shawnbot · 2017-04-07T16:19:15Z

👀 Preview on Federalist

⚠️ ES5 users who reference submodules directly will need to transpile their JavaScript when this is released. For instance, if you require('uswds/src/js/components/accordion') and bundle your JavaScript with browserify or webpack, you'll need to incorporate the respective Babel plugin (babelify or babel-loader, respectively). Users of React or Angular 2 do not need to do anything special.

This PR fixes #1423 by replacing all of our "direct" event listeners (added to individual elements once, on DOM ready) with delegated listeners on the <body> that should work regardless of what happens in the rest of the DOM. I've created standalone React and Angular 1 test cases that reference the JS built by this branch, and both appear to work well. Here's the lowdown:

All components are implemented as behaviors with additional on() and off() methods that add and remove all of the declared listeners to the <body> on DOM ready (in the respective initializers). Our wrapped behavior provides a couple of additional niceties:
If a behavior has an init() method, that will be run by the behavior's on() method automatically (before listeners are added) to initialize any relevant elements. XXX maybe this should be called setup()?
If a behavior has a teardown() method, that will be run by the behavior's off() method automatically, to clean up any temporary state.

The upside of this is that if you (the user of the Standards) render parts, or even the entirety, of your page's content from an AJAX request or with another framework, you don't have to worry about event listeners being blown away and having to add them again.

Along the way, I've basically refactored all of the JS from the ground up. Here's what else happened:

The "main" script has been renamed from start.js to uswds.js, to better match the destination filename.
The main script now exports a uswds object, which currently has two properties:
- prefix is the usa prefix used to derive CSS selectors for each component behavior.
- components is an object of named component behaviors, each of which can be added (add()/on()) and removed (remove()/off()) to any element root at runtime.
The components/x.js + initializers/x.js duo of files for each component has been replaced by a single "behavior" script in components/x.js. In other words, component definitions are behaviors.
Some functions that lived in the components directory have been converted into stateless functions (toggleFieldMask(), etc.) and moved to utils.
Some ambiguously named component scripts (such as forms.js) have been replaced with more specific ones (such as password.js).
The "password" component (used in the password reset and sign in form templates) now allows you to supply a data-hide-text attribute to specify what's displayed when you toggle it, and derives a sensible default by replacing "Show" with "Hide" in the initial text content (respecting case) if there is none. Previously, these values were hard-coded in JavaScript.
All of the ES5 shimming is now done with third-party npm modules, such as array-foreach and array-filter.
There's a new, better ponyfill for HTML5 dataset.
All of the scripts in src/js/vendor are gone now. Those were a relic of the site, AFAICT.

Unit test improvements

Along the way, I've also made some improvements to our unit test suite and removed some unnecessary dependencies:

There are now unit tests for the poorly documented (big) footer faux-accordion and skip-nav tabindex switching, which fixes Clean up focus outlines for main content areas uswds-site#238.
Our unit test suite now uses the Node.js built-in assert module instead of should.
We have officially removed jQuery from all of our unit tests. 🎉
Unit tests that previously imported HTML templates as JS now read actual HTML files as strings. Eventually, these files may be generated from the Fractal templates, or tests may be run on Fractal's HTML output.

ES2015

I've also taken the liberty of upgrading the component JS to ES2015 (ES6) and incorporating Babel into our builds. This makes our source JS a lot more concise, and at a fairly insignificant size penalty:

File	Before	After
`uswds.js`	104K	152K
`uswds.min.js`	16K	20K
`uswds.min.js.map`	100K	128K

toolness · 2017-04-07T21:57:01Z

Cool!

I'm not very familiar with how the USWDS JS components currently work, so I guess one question I have is, how do the virtual DOM interop concerns you have for this PR not also apply to the current state of the JS, outside of this PR?

I've used third-party code with React a bit (see the lots of legacy section of the CALC React migration notes) but the main way I've evaluated whether to rewrite a dynamic component in React vs. use a native DOM implementation has largely centered around how "encapsulated" the component is in the DOM.

For example, with CALC, the d3 histogram was an easy decision to make because it's easy to just take that one DOM node and tell React "let me manage this DOM node, don't mess with it". It's just a black box to React, and we can just subscribe to React's component lifecycle methods to make the native DOM changes required when the component's props are changed, remove any event listeners when the component leaves the DOM, and so on.

On the other hand, things might get more complicated if there's lots of interleave between DOM and virtual DOM components. An example of this might be a native DOM-based accordion component, where the "frame" of the accordion is managed by native DOM code, but you want content inside the accordion panels that's managed by a virtual DOM. This is something I don't have much experience with, so it might be possible if you're careful, but it's also where I'd start seriously considering just re-implementing the native DOM code in React, to avoid any possibility of things exploding unexpectedly. Again, though, I don't have a lot of experience with this kind of situation, so I could be wrong.

shawnbot · 2017-04-07T23:32:41Z

Thanks for taking a look, @toolness! I took some time this afternoon to make a little React app to test out this approach, and it appears to work. Check it out. Only the gray section is actually rendered with React (every time you click one of the two buttons), but it works the way I'd expect it to: if you open an accordion with delegated listeners then re-render the DOM with React, it doesn't change the aria-expanded and aria-hidden attributes that control the visibility of the sections.

This is the first time I've really used React, but it seems to me that users can either go this way (using the Standards JS) or re-implement the accordion interactivity in React, which seems like it might actually be pretty straightforward.

Update: you can also peep the code for the React app.

shawnbot · 2017-04-07T23:37:52Z

Okay, so this React app actually turns out to be a pretty decent test of all the components I've upgraded to use event delegation. Everything outside of the React-rendered accordion is vanilla JS and appears to work well. The tests pass in jsdom, too!

Time to test out Angular?

joshbruce · 2017-04-18T00:08:17Z

Wow! @shawnbot you've been busy. Not sure why I just now got the email on this. Having issues with local development and things...gonna tag in some teammates to see if they can help out. Having said that, it might be moot at this point.

shawnbot · 2017-04-18T00:12:03Z

Okay, so as of c8085f8 I think the polyfill/ponyfill situation is much better: I've replaced our home-brewed DOM ready and dataset implementations with domready and elem-dataset, respectively (which eliminates the reduce() call, too); the classList/DOMTokenList polyfill is back, and I've kept the hidden polyfill because it's so dead-simple and I couldn't find a 3rd-party replacement that wasn't much larger. As it stands, the last two are the only changes that our JS makes to the global environment, which is good practice.

I've also renamed uswds.js back to start.js, and updated package.json accordingly in f6dc769.

shawnbot · 2017-04-18T00:12:13Z

@joshbruce thanks! FYI, you and your team can test this branch by npm installing from git, a la:

npm install --save "uswds@https://github.com/18F/web-design-standards.git#feature-event-delegation"

jseppi

This is looking real nice, @shawnbot!

Minor aside: any reason you're using a custom eslint config instead of eslint-config-airbnb-base? I personally think it would be good to use that config since it is g-frontend-approved :)

jseppi · 2017-04-18T15:01:43Z

src/js/utils/behavior.js

+const forEach = require('array-foreach');
+const Behavior = require('receptor/behavior');
+
+const sequence = function () {


I think you could use the spread operator to write this line as

const sequence = function(...seq) {

and remove the following line (const seq = [].slice.call(arguments);

Same, re: ES2015 and Node v6.

But the es2015 babel config you're using with babelify will take care of this :)

That's true, but our mocha/jsdom tests require() the modules directly. ☹️

😬 Perhaps explore using jest and its babel runner: https://github.com/facebook/jest#using-babel

jseppi · 2017-04-18T15:03:41Z

src/js/utils/behavior.js

+
+const sequence = function () {
+  const seq = [].slice.call(arguments);
+  return function (target) {


Similarly, you could use ES2015 default args to write this as

return function (target=document.body) {

Same, re: ES2015 and Node v6.

jseppi · 2017-04-18T15:22:12Z

src/js/utils/behavior.js

@@ -0,0 +1,31 @@
+'use strict';
+const assign = require('object-assign');


Consider using import in place of all requires. While it wouldn't really have any immediate effect, it would pave the way for tree-shaking in your bundler (either webpack2 or rollup).

Could also obviously be handled later in a separate PR.

Yeah, we're still using Node 6, so I was sticking to the subset of ES2015 supported natively. Let's tackle this in a future PR. :)

jseppi · 2017-04-18T15:23:59Z

src/js/components/accordion.js

 };

-module.exports = Accordion;
+module.exports = behavior({


Along the same vein of using import, consider using export/export default. Perhaps in a separate PR.

jseppi · 2017-04-18T15:25:34Z

src/js/components/banner.js

-}
+const toggleBanner = function (event) {
+  event.preventDefault();
+  this.closest(HEADER).classList.toggle(EXPANDED_CLASS);


Is closest polyfilled? I think it needs to be for IE. http://caniuse.com/element-closest

Yeah, it's actually polyfilled by receptor via element-closest, which also polyfills matches(). (Which I guess means that we're not necessarily playing nice in that respect...)

jseppi · 2017-04-18T15:28:53Z

src/js/components/search.js

-    deactivateDispatcher.off();
-  }
-}
+const CLICK = ('ontouchstart' in document.documentElement)


consider extracting CLICK since it is used in multiple places

jseppi · 2017-04-18T15:30:47Z

src/js/components/search.js

+    if (input) {
+      input.focus();
+    }
+    const listener = ignore(


explanatory comment would be good here

jseppi · 2017-04-18T15:34:22Z

src/js/components/skipnav.js

+      target.setAttribute('tabindex', -1);
+    }));
+  } else {
+    // throw an error?


Hrm, might want a single strategy for dealing cases where an expected element is missing. I see some other cases where an error is thrown: https://github.com/18F/web-design-standards/pull/1836/files#diff-6449513e1e61be79d366e960d8578ffbR16

I don't have a good feeling for what is "correct." On one hand, you would help developers by making problems obvious. On the other, it might break otherwise functional sites, which could be nasty for users.

Yeah, I'm on the fence about this one. In other cases (e.g. with aria-controls, it doesn't feel like an error if no elements resolve because it's theoretically possible to have a button that toggles just its own state. But in this case, the link should resolve, and if it doesn't, that's at least an accessibility error.

After looking at this a bit more closely, I'm going to leave it as-is for now. Throwing an error seems like the right thing to do, but that would effectively stop the link from working, and I'm a little weary that just doing link.getAttribute('href').slice(1) is naive. I'd rather users just don't get the tabindex behavior because their link is malformed (and the link still works, possibly) than preventing the link from being followed altogether.

jseppi · 2017-04-18T15:34:56Z

src/js/components/skipnav.js

+};
+
+module.exports = behavior({
+  'click': {


Why not same touchstart || click as used in other components?

Very good question :)

shawnbot · 2017-04-18T16:34:02Z

Thanks for the review, @jseppi! I agree that we should be using the guild-approved config; let's tackle that in a future PR, too. In the meantime, I'll fix up some of the other stuff you raised above.

shawnbot · 2017-04-18T16:38:39Z

Also: I did a quick manual test of the accordion in IE9 on Sauce Labs, and... it works! 💥

shawnbot · 2017-04-18T18:36:58Z

Okay @jseppi, I think I'm ready for a final pass.

I addressed all of your comments above (saving more advanced ES6 features for later, normalizing all click events, etc.), tests pass, and it looks good in Fractal. I even added both a Fractal variant for the multiselectable accordion (won't be available until Federalist finishes the build) and some additional unit tests in 44ac51e.

This removes the redundant 'title' template context and reduces the 'package' to just name and version, so as not to spam the Context pane of individual components with irrelevant data.

jseppi

🎉 💃 🕺 🎉
This looks good to me!

shawnbot · 2017-04-19T23:52:22Z

Here we go... 🤞

Shawn Allen added 24 commits April 7, 2017 00:31

add receptor

404f264

add receptor.behavior() wrapper

12927f3

accordion template => HTML

e0bcab9

comments

8ced2e0

add toggle() function

595c61f

refactor accordion to use event delegation

8a9b6e5

clean up accordion unit tests

e1e98fa

simplify

67310f8

upgrade nav behavior to use event delegation

1b54b3d

eslint: enable es6

03ca4fb

linting

67e85b4

add support for behavior.teardown()

af26803

better search spec

061ced1

migrate search component to event delegation

4bd9d22

use event delegation on banner; add tests

fe9b9ad

move CLICK declaration

7631a47

babelify that JS

3514749

suggest Node v6.x

bf7beb9

shuffle deps

68cc777

fix select() args order

3743780

don't always run eslint

33fe76d

always attach behaviors to <body>

eec17f5

add nav_prefix so all headers can work in tandem

3f91b43

simplify some more

26d10d6

shawnbot requested review from rogeruiz and msecret April 7, 2017 16:19

shawnbot added the [Status] Accepted label Apr 7, 2017

Shawn Allen added 3 commits April 17, 2017 16:56

rename uswds.js => start.js

b0dc9e0

fix path to browserify entry point

3293aaa

replace dataset ponyfill with elem-dataset from npm

c8085f8

jseppi reviewed Apr 18, 2017

View reviewed changes

Shawn Allen added 8 commits April 18, 2017 10:19

add another search form test

d5dfce5

better comments for search events

b0a95b9

move click/touchstart definition to events.js

a6e32c3

replace 'click' with CLICK events

8db7e43

move link selector to the top

c50f7e2

add Fractal variant for multiselectable accordion

6831736

make accordion tests more robust; add multiselectable test

44ac51e

"multi-selectable" => "multiselectable"

89ad177

clean up fractal.js

9b09204

This removes the redundant 'title' template context and reduces the 'package' to just name and version, so as not to spam the Context pane of individual components with irrelevant data.

jseppi approved these changes Apr 18, 2017

View reviewed changes

shawnbot changed the title ~~[WIP] Implement event delegation; refactor JS and tests~~ Implement event delegation; refactor JS and tests Apr 18, 2017

shawnbot merged commit 5cf8e96 into develop Apr 19, 2017

shawnbot removed the [Status] Accepted label Apr 19, 2017

shawnbot deleted the feature-event-delegation branch April 24, 2017 16:42

This was referenced Apr 24, 2017

Update JS bundling guidance post-refactor #1879

Closed

Document "validator" component behavior #1432

Closed

[UI component]: Abstract accordion JS #1363

Closed

Decide how to communicate JS refactoring in our next release #1885

Closed

toolness mentioned this pull request Jun 24, 2017

Actually use validator component. #1992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement event delegation; refactor JS and tests #1836

Implement event delegation; refactor JS and tests #1836

shawnbot commented Apr 7, 2017 •

edited

Loading

toolness commented Apr 7, 2017

shawnbot commented Apr 7, 2017 •

edited

Loading

shawnbot commented Apr 7, 2017 •

edited

Loading

joshbruce commented Apr 18, 2017 •

edited

Loading

shawnbot commented Apr 18, 2017

shawnbot commented Apr 18, 2017

jseppi left a comment

jseppi Apr 18, 2017

shawnbot Apr 18, 2017

jseppi Apr 18, 2017

shawnbot Apr 18, 2017

jseppi Apr 18, 2017

jseppi Apr 18, 2017

shawnbot Apr 18, 2017

jseppi Apr 18, 2017

jseppi Apr 18, 2017

shawnbot Apr 18, 2017

jseppi Apr 18, 2017

jseppi Apr 18, 2017

shawnbot Apr 18, 2017

jseppi Apr 18, 2017

jseppi Apr 18, 2017

jseppi Apr 18, 2017

shawnbot Apr 18, 2017

shawnbot Apr 18, 2017

jseppi Apr 18, 2017

shawnbot Apr 18, 2017

shawnbot commented Apr 18, 2017

shawnbot commented Apr 18, 2017

shawnbot commented Apr 18, 2017

jseppi left a comment

shawnbot commented Apr 19, 2017

		@@ -0,0 +1,31 @@
		'use strict';
		const assign = require('object-assign');

Implement event delegation; refactor JS and tests #1836

Implement event delegation; refactor JS and tests #1836

Conversation

shawnbot commented Apr 7, 2017 • edited Loading

Unit test improvements

ES2015

toolness commented Apr 7, 2017

shawnbot commented Apr 7, 2017 • edited Loading

shawnbot commented Apr 7, 2017 • edited Loading

joshbruce commented Apr 18, 2017 • edited Loading

shawnbot commented Apr 18, 2017

shawnbot commented Apr 18, 2017

jseppi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbot commented Apr 18, 2017

shawnbot commented Apr 18, 2017

shawnbot commented Apr 18, 2017

jseppi left a comment

Choose a reason for hiding this comment

shawnbot commented Apr 19, 2017

shawnbot commented Apr 7, 2017 •

edited

Loading

shawnbot commented Apr 7, 2017 •

edited

Loading

shawnbot commented Apr 7, 2017 •

edited

Loading

joshbruce commented Apr 18, 2017 •

edited

Loading