I'm practicing by writing utilities for myself; I prefer CSS3 transitions over jQuery animations whenever possible. This is intended to be used for transitioning multiple DOM elements who's relationship to one and other is only given by the context (ie. on clicking a menu icon, hide/show, relocate, resize, etc. multiple dynamic-content elements).
I'm seeking advice related to design, utility, proper use of JavaScript concepts, etc. i.e. does it make sense for this to exist, what needs are overlooked or problems left unresolved, etc.
Note: Ignore client-compatibility; assume well-configured modernizr is loaded.
function obj_merge(keys_obj, vals_obj) {
if (!is_object(keys_obj) || !is_object(vals_obj)) throw "keys_obj and vals_obj must both be javascript objects";
var merged_obj = {};
for (var key in keys_obj) merged_obj[key] = key in vals_obj ? vals_obj[key] : keys_obj[key];
for (var key in vals_obj) if ( !(key in merged_obj) ) merged_obj[key] = vals_obj[key];
return merged_obj;
}
function eCustom(eName, eProperties) {
var defaultProps = {"bubbles": true, "cancelable": false, "eventPhase": 0, "type": eName};
if (typeof(eProperties) == "object") {
for (var prop in eProperties) {
if (eProperties.hasOwnProperty(prop)) {
defaultProps[prop] = eProperties[prop];
}
}
}
return jQuery.Event(eName, defaultProps);
}
function is_object(obj) { return typeof obj === 'object' && obj != null;}
function is_array(obj) { return obj instanceof Array; }
function EffectChain(config) {
var effect_chain = Object();
effect_chain.defaults = {
interval:undefined,
context: undefined
};
effect_chain.step_complete = eCustom("TransitionerStepComplete");
effect_chain.steps = [];
effect_chain.step_template = {
target: false,
context: "body",
state: undefined,
interval: undefined,
next: undefined
}
effect_chain.exec_result = undefined;
effect_chain.init = function(config) {
$(this).on("TransitionerStepComplete", function(e, data) {this.exec_step(data.step_index, data.target)})
if (is_object(config) ) this.defaults = obj_merge(this.defaults, config);
for (var key in this.defaults) if (this.defaults[key]) this.step_template[key] = this.defaults[key];
return this;
}
effect_chain.add = function(step) {
if ( is_array(step) ) {
while (step.length > 0) this.add(step.shift());
return this;
}
step = obj_merge(this.step_template, step);
step.exec = function(self, target) {
if (!target) target = self.target;
var f = function() {
if ("attr" in self.state) {
for (var attr in self.state.attr) $(target, self.context).attr(attr, self.state.attr);
}
if ("add" in self.state) {
if ( !is_array(self.state.add) ) self.state.add = [self.state.add];
for (var i=0; i < self.state.add.length; i++) {
try { $(target).addClass(self.state.add[i](target)); }
catch(e) { $(target).addClass(self.state.add[i]); }
}
}
if ("remove" in self.state) {
if ( !is_array(self.state.remove) ) self.state.remove = [self.state.remove];
for (var i=0; i < self.state.remove.length; i++) {
try { $(target).removeClass(self.state.remove[i](target)); }
catch(e) { $(target).removeClass(self.state.remove[i]); }
}
}
};
$(target, self.context).each(f);
if ( self.next ) {
try { return self.next(target, self.context); } // test: next is a function
catch(e) {
try { // test: next is range or specific index of selected elements
try { var refer_el = $(self.next.selector, context); }
catch(e) { var refer_el = $(self.next.selector); }
try { return refer_el.slice(self.next.from, "to" in self.next ? self.next.to : -1) }
catch(e) { return refer_el[self.next.index]; }
} catch(e) {
return $(self.next); // test: next is a string selector
}
}
}
return;
}
this.steps.push(step);
return this;
};
effect_chain.exec_step = function(step_index, target) {
var self = this;
var step = this.steps[step_index]
var result = step.exec(step, target);
step_index++;
if (this.steps.length > step_index) {
setTimeout( function() {
$(self).trigger("TransitionerStepComplete", {step_index: step_index, target:result})
}, step.interval);
} else {
this.exec_result = result;
}
}
effect_chain.duration = function() {
var t = 0;
// sum all step intervals except last
for (var i=0; i < this.steps.length - 1; i++) t += this.steps[i].interval;
return t;
}
effect_chain.run = function(initial_target) { this.exec_step(0, initial_target); }
effect_chain.init(config);
return effect_chain;
}
1 Answer 1
Brief review for now - I'll try to take a closer look when I have more time.
Consistency and naming
JavaScript, by convention, usescamelCase
(andPascalCase
for constructors). You mix in a lot ofsnake_case
, which, frankly, is plain weird. E.g.EffectChain
follows the conventions (presuming you call it withnew
, which you can, despite it not being a "true" constructor), but its internals do not. Why?Meanwhile you have
eCustom
, which is properly cased for a function name - but it's just not a great name. Why notcreateCustomEvent
? Be descriptive and excessively abbreviate things.Line length and missing braces
I usually recommend always using braces even for one-line blocks. But here, I'd doubly recommend it. You seem hell-bent on writing as much as possible on a single line, going so far as defining top-level functions that way (though they have braces, it's still all on a single line). It makes everything harder to read, if you ask me. Let the code breathe a little.Lines like these are just no fun to make sense of:
for (var key in keys_obj) merged_obj[key] = key in vals_obj ? vals_obj[key] : keys_obj[key]; for (var key in vals_obj) if ( !(key in merged_obj) ) merged_obj[key] = vals_obj[key];
One-line loop with a ternary, followed by a one-line loop with a one-line
if
? Jeez. Linebreaks and braces don't cost nuthin', so use 'em.Wheel re-invention
jQuery already has a number of type-checking methods, and a "merge" function in the form of$.extend
so there's no reason to invent your own functions for all that.
-
\$\begingroup\$ Many or most of the style choices are basically PEP8 habits, but the insight is appreciated insofar as it will help when working with other JS authors. Also wasn't familiar with some of those jQuery methods. I really do appreciate these comments, but, if you do indeed get a chance to "take a closer look", I'm really seeking a more "zoomed out" analysis; more to do with the validity of what this accomplishes and how it's accomplished as opposed to the techniques and styles in writing it. \$\endgroup\$Jonline– Jonline2015年06月19日 13:55:14 +00:00Commented Jun 19, 2015 at 13:55
-
\$\begingroup\$ Also I am quite willing to go through the code and make it more readable, add comments, etc., especially if it's particularly unclear as to what any part is meant to achieve—tell me how to help! :) \$\endgroup\$Jonline– Jonline2015年06月19日 13:59:23 +00:00Commented Jun 19, 2015 at 13:59
-
\$\begingroup\$ @Jonline roger that. i'll try to take a look this weekend. So far i've just been too busy. \$\endgroup\$Flambino– Flambino2015年06月19日 14:03:35 +00:00Commented Jun 19, 2015 at 14:03
-
\$\begingroup\$ Oh, one other thing—" E.g. EffectChain follows the conventions (presuming you call it with new, which you can, despite it not being a "true" constructor), but its internals do not. Why?" Because I have no idea what a "true" constructor is in a JavaScript context, I've tried to understand this before but I've always either been confused or—worse—thought I'd understood and then created things that didn't work. Not on you to teach, but if you happen to have a recommended source for grasping this it'd be welcome! \$\endgroup\$Jonline– Jonline2015年06月19日 14:07:53 +00:00Commented Jun 19, 2015 at 14:07
-
\$\begingroup\$ I've edited the code pretty heavily, still primarily snake case haha, but should be much more readable now. \$\endgroup\$Jonline– Jonline2015年06月19日 15:10:37 +00:00Commented Jun 19, 2015 at 15:10
EffectChain
objects would see enough reuse to warrant the amount of code that has to be written, but it at least demonstrates use: codepen.io/anon/pen/PqEBxm \$\endgroup\$