Update JS to ES6 #660

anselmbradford · 2017-10-18T16:21:55Z

Changes

Update cf-tables and cf-expandables to ES6 syntax.
Remove "Organism" suffix from table and expandable names (the type should be able to be looked up through the prototype, also this is obfuscated through minification anyway).
Prefer named methods vs anonymous functions.

Testing

https://github.com/cfpb/capital-framework/blob/canary/CONTRIBUTING.md#testing-components-locally expandables and tables should still work like the live site.

jimmynotjim · 2017-10-19T15:22:38Z

src/cf-expandables/src/Expandable.js

+const contains = domClassList.contains;
+const addClass = domClassList.addClass;
+const closest = require( 'cf-atomic-component/src/utilities/dom-closest' ).closest;
+const removeClass = domClassList.removeClass;


Nitpicky, but lets group all the domClassList child vars together.

jimmynotjim · 2017-10-19T15:24:47Z

src/cf-expandables/src/Expandable.js

-    this.transition.toggleExpandable();
-    this.toggleTargetState( this.ui.target );
-  },
+  transition: null,


Should these values down to L:43 be left aligned to stay consistent with the objects above?

jimmynotjim · 2017-10-19T15:27:11Z

src/cf-expandables/src/Expandable.js

+function initialize() {
+  const customClasses = {
+    BASE_CLASS:   'o-expandable_content__transition',
+    EXPANDED:     'o-expandable_content__expanded',


EXPANDED and COLLAPSED are defined in the classes object. Should they be saved as constants at the top of the file?

Hmm, that's a good point! Let's revisit that in #665, lest I break some existing functionality here.

jimmynotjim · 2017-10-19T15:29:22Z

src/cf-expandables/src/Expandable.js

+  const transition = new ExpandableTransition( this.ui.content, customClasses );
+  this.transition = transition.init();
+
+  const groupElement = closest( this.ui.target, '.' + this.classes.groupAccordion );


Outside the scope, but if we're deciding all expandables should be in a group organism, should we target and initialize on the group instead of the individual expandables? It might make this a bit more efficient because we wouldn't have to lookup the group a whole bunch of times.

Totally! I think everything would move to the group so we could eliminate the closest helper. (Related issue #665)

jimmynotjim · 2017-10-19T20:47:24Z

src/cf-tables/src/TableRowLinks.js

-};
+  target = closest( event.target, 'tr' );
+  const link = target.querySelector( 'a' );
+  if ( link ) window.location = link.getAttribute( 'href' );


This should be wrapped in curly braces to match the return on L:31

jimmynotjim · 2017-10-19T20:49:20Z

src/cf-tables/src/TableSortable.js

+ * Function used to create computed and triggered properties.
+ */
+function initialize() {
+  this.sortClass = UNDEFINED;


Should these be part of a customClasses object like in the Expandables code instead of properties?

This looks like a different use case. In Expandables there's a set of class names that are assigned to constants that are only used inside the initialize method, so their scope is fine. In this case, sortClass is not constant and is used across methods, so is assigned as a property to share between methods. Personally I prefer not to expose anything publicly that is not needed in the public API, lest it leads to unintended implementations, so if sortClass could be internal that would be better (such as by moving it's location above the scope of the TableSortable object), but I don't think that's currently possible here as the methods here get added to instances of the Table organism, so that if sortClass was in a higher scope it would then become shared among all Table organisms using the TableSortable modifier. Is that right @sebworks?

yep, that is correct @anselmbradford

Hmm, sidenote that my renaming these capitalized like they are classes when they're not instantiating new instances is possibly misleading.

sebworks

nice work

anselmbradford requested review from Scotchester, jimmynotjim, mistergone and sebworks October 18, 2017 16:21

jimmynotjim reviewed Oct 19, 2017

View reviewed changes

jimmynotjim approved these changes Oct 20, 2017

View reviewed changes

sebworks reviewed Oct 20, 2017

View reviewed changes

anselmbradford force-pushed the es6_update branch from bd54243 to d5fd939 Compare October 20, 2017 19:00

Update JS to ES6

cd25899

anselmbradford force-pushed the es6_update branch from d5fd939 to cd25899 Compare October 20, 2017 19:01

anselmbradford merged commit e614de9 into canary Oct 20, 2017

anselmbradford deleted the es6_update branch October 20, 2017 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update JS to ES6 #660

Update JS to ES6 #660

anselmbradford commented Oct 18, 2017

jimmynotjim Oct 19, 2017

anselmbradford Oct 20, 2017

jimmynotjim Oct 19, 2017

anselmbradford Oct 20, 2017

jimmynotjim Oct 19, 2017

anselmbradford Oct 20, 2017

jimmynotjim Oct 19, 2017

anselmbradford Oct 20, 2017

jimmynotjim Oct 19, 2017

anselmbradford Oct 20, 2017

jimmynotjim Oct 19, 2017

anselmbradford Oct 20, 2017

sebworks Oct 20, 2017

anselmbradford Oct 20, 2017

sebworks left a comment

Update JS to ES6 #660

Update JS to ES6 #660

Conversation

anselmbradford commented Oct 18, 2017

Changes

Testing

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

sebworks left a comment

Choose a reason for hiding this comment