HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

eDomForm - dynamic forms without writing any JavaScript

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withoutjavascriptanywritingedomformdynamicforms

Problem

I'd like some comments and any suggestions that you have for where to go next with this.

I've been working on developing a JavaScript library that allows for dynamic forms by simply adding some extra HTML tags and attributes to your form. It's brand new, so it doesn't have a lot of features, but it's based on the idea that you shouldn't need to write a bunch of JavaScript and have to deal with bugs and stuff just to get your form to have multiple pages or to add new fields when you click a button... stuff like that.

All you have to do to make it work is include jQuery and this JavaScript in the page after all the form HTML.

...


I've got the current JavaScript in a Fiddle with some example HTML and CSS, and some basic documentation/guide material in a Google doc that you can comment on if you like. Any comments and suggestions are welcome.

You're also welcome to use this for whatever you want as-is or with any modifications you or anyone comes up with.

Fiddle

eDomForm

```
//Configuration variables
var config = jQuery("edfconfig");

function getConfigBoolean(attr) {
if (config != null)
return jQuery(config).attr(attr) != undefined;
}
function getConfigValue(attr) {
if (config != null)
return jQuery(config).attr(attr);
}

var noasterisks = getConfigBoolean("noasterisks");
var addafter = getConfigBoolean("addafter");
var doactions = getConfigBoolean("doactions");
var noappend = getConfigBoolean("noappend");
var requiredmessage = getConfigValue("requiredmessage");
var requiredmessageid = getConfigValue("requiredmessageid");

//eDomForm variables
var attrs = jQuery("edfvars")[0];
var variables = {};
if (attrs != null)
for (i=0;iBack Next');
});

//Next and Back buttons
var pages = jQuery(".form_page");
var backButtons = jQuery(".back");
var nextButtons = jQuery(".next");
for (i=0;i -1;
}

var aliases = jQuery(".alias");

for (i=0;i -1) {
var nums = connections[key].split("

Solution

On the whole

Interesting, from a philosophy perspective, you are now moving from code with potential JavaScript bugs to code with potential HTML 'configuration' bugs. Debugging configuration bugs is annoying since there is no dedicated tooling for it.

Furthermore, you have some interesting design choices HTML wise;

  • You have custom tags but you are not using registerElement



  • You have custom properties that are not compatible with $().data()



  • This works, but it should be avoided when possible, and I believe it is possible



From a consistency perspective;

  • You have a tag property requiredmessage



  • You have classes required_empty and required_field



  • Your code is lowerCamelCase



I would synchronize everything to lowerCamelCase across properties, CSS classes and code, otherwise users will have to keep referring to documentation to see what the proper spelling is.

Coding*

Constants, most of these should have a well named constant name;

  • You use ".required_field" and "required_field" several times



  • You use ".alias" and "alias" several times



  • etc. etc.



Comments:

  • handleSwitchers



  • Other than that, the commenting throughout the code is very uneven



Some jQuery specific items:

-
This:

jQuery(".form_page").each(function(i){
  jQuery(this).prepend('Back Next');
});


can be

var buttons = 'Back Next';
jQuery(".form_page").prepend( buttons );


basically prepend works on every selected element.

-
This:

for (i=0;i<pages.length;i++) {
    if (i != pages.length-1) {
        nextButtons[i].onclick = function() {
            jQuery(this).closest("div").fadeOut();
            jQuery(this).closest("div").nextAll(":not(.disabled):first").fadeIn();
        };
    } else {
        jQuery(nextButtons[i]).remove();
    }
    if (i != 0) {
        backButtons[i].onclick = function(i) {
            jQuery(this).closest("div").fadeOut();
            jQuery(this).closest("div").prevAll(":not(.disabled):first").fadeIn();
        };
    } else {
        jQuery(backButtons[i]).remove();
    }
}


is basically trying to make every back and next button work, and remove the first back button and the last next button, the following code does the exact thing with fewer lines and more clarity of purpose:

nextButtons.last().remove();  
backButtons.first().remove();  

nextButtons.click( function(){
  $(this).closest("div").fadeOut()
                      .nextAll(":not(.disabled):first").fadeIn();         
});

backButtons.click( function(){
  $(this).closest("div").fadeOut()
                        .prevAll(":not(.disabled):first").fadeIn();     
})


Note also that I chained the two statement within the click handlers for efficiency.

  • You use jQuery everywhere, but not here: var required_fields = document.getElementsByClassName("required_field"); why ?



  • Cache jQuery(this) into $this = jQuery(this) in all your listeners where you access jQuery(this) more than once.



-
When you decide to add a class or remove a class based on a Boolean, consider toggleClass, this:

jQuery(this).click(function(){
  if (jQuery(this).is(":checked")) {
    jQuery("."+page).removeClass("disabled");
  } else {
    jQuery("."+page).addClass("disabled");
  }
});


could be

jQuery(this).click(function(){
  var $this = jQuery(this);
  jQuery("."+page).toggleClass( 'disabled' , !$this.is(":checked") );
});


-
Not really jQuery related, but realize that ternaries can be your friend:

required[i].onblur = function() {
if (this.value == "") {
        jQuery(this).next().css("color","#f00");
    } else {
        jQuery(this).next().css("color","#000");
    }
};


can be ( you can put the assignment outside of the loop )

required.blur( function() {
    jQuery(this).next().css("color", (this.value == "") ? "#f00" : "#000");    
});


When you think about it, "#f00" should be a constant, and probably you should assign a class instead of putting the color red.

Code Snippets

jQuery(".form_page").each(function(i){
  jQuery(this).prepend('<button type="button" class="back">Back</button> <button type="button" class="next">Next</button>');
});
var buttons = '<button type="button" class="back">Back</button> <button type="button" class="next">Next</button>';
jQuery(".form_page").prepend( buttons );
for (i=0;i<pages.length;i++) {
    if (i != pages.length-1) {
        nextButtons[i].onclick = function() {
            jQuery(this).closest("div").fadeOut();
            jQuery(this).closest("div").nextAll(":not(.disabled):first").fadeIn();
        };
    } else {
        jQuery(nextButtons[i]).remove();
    }
    if (i != 0) {
        backButtons[i].onclick = function(i) {
            jQuery(this).closest("div").fadeOut();
            jQuery(this).closest("div").prevAll(":not(.disabled):first").fadeIn();
        };
    } else {
        jQuery(backButtons[i]).remove();
    }
}
nextButtons.last().remove();  
backButtons.first().remove();  

nextButtons.click( function(){
  $(this).closest("div").fadeOut()
                      .nextAll(":not(.disabled):first").fadeIn();         
});

backButtons.click( function(){
  $(this).closest("div").fadeOut()
                        .prevAll(":not(.disabled):first").fadeIn();     
})
jQuery(this).click(function(){
  if (jQuery(this).is(":checked")) {
    jQuery("."+page).removeClass("disabled");
  } else {
    jQuery("."+page).addClass("disabled");
  }
});

Context

StackExchange Code Review Q#27854, answer score: 2

Revisions (0)

No revisions yet.