3
\$\begingroup\$

Right now I'm mixing JS and jQuery. I would like to find one unique solution with JS only possibly.

How can I organize the code according to best code practices?

Another thing I need to add is a check at the beginning of the function. If the classes are there nothing should happen, if not there then add them just as below.

function changeClass() {
    document.getElementById("message").className += " dropzone-wrap";
    document.getElementById("dragNdrop").className += " dropzone-content";
}

window.onload = function() {
    document.getElementById("ToggleTxTDrop").addEventListener( 'click' , changeClass );
}

/* lets remove the previous classes when on focus */
$('#message').on('focus', function(){
    $(this).removeClass('dropzone-wrap');
    $(this).parent().removeClass('dropzone-content');
})
\$\endgroup\$
  • 1
    \$\begingroup\$ I'll just... drop this here: youmightnotneedjquery.com \$\endgroup\$ – Vogel612 Jun 12 '15 at 21:38
  • \$\begingroup\$ I think that's really helpful however I dont know how to refactor my code above. Any help in refactor it using pure Java Script I would really appreciate it. \$\endgroup\$ – user2513846 Jun 13 '15 at 0:44
4
\$\begingroup\$

Here's a step-by-step conversion from your code to the JS-only code of youmightnotneedjquery.com.

function changeClass() {
    document.getElementById("message").className += " dropzone-wrap";
    document.getElementById("dragNdrop").className += " dropzone-content";
}

Your method here can only ever be used to add "dropzone-wrap" and "dropzone-content". Consider a different approach, where you provide the DOM Element and CSS Classes as parameters.

Consider two such methods - one to add the class (from $.addClass()), and one to remove it ($.removeClass()).

// http://youmightnotneedjquery.com/#add_class
function addClass(el, className) {
    if (el.classList)
      el.classList.add(className);
    else
      el.className += ' ' + className;
}

// http://youmightnotneedjquery.com/#remove_class
function removeClass(el, className) {
    if (el.classList)
      el.classList.remove(className);
    else
      el.className = el.className.replace(new RegExp('(^|\\b)' + className.split(' ').join('|') + '(\\b|$)', 'gi'), ' ');
}

Note that I'd remove the class before adding it, as that code does not protect against long repeated classname strings.

Anyway, let's continue with your code:

window.onload = function() {
    document.getElementById("ToggleTxTDrop").addEventListener( 'click' , changeClass );
}

Using onload works, but it isn't the right way to do it. With jQuery this should be $(window).on('load', function() { ... });... with native Javascript, it isn't much different (.addEventListener).

For the function code itself, create a handler method that uses the addClass() abstraction method to replace your old changeClass() functionality:

function txtDropClicked(evt) {
    addClass(document.getElementById('message'), 'dropzone-wrap');
    addClass(document.getElementById('dragNdrop'), 'dropzone-content');
}

window.addEventListener('load', function() {
    document.getElementById("ToggleTxTDrop").addEventListener( 'click' , txtDropClicked );
});

Now we have your classes being added on click; let's update the onfocus handler in the same fashion:

/* lets remove the previous classes when on focus */
function removeClassesOnFocus(evt) {
    // evt.target is the element that triggered the event. We bound to #message.
    removeClass(evt.target, 'dropzone-wrap');
    removeClass(evt.target.parentNode, 'dropzone-content');
}

// this bit really should wait for `window.load` too.
document.getElementById('message').addEventListener('focus', removeClassesOnFocus);

If the classes are there nothing should happen

To address that, the removeClassesOnFocus function could be reworked to look up the element if it isn't provided.

function txtDropClicked(evt) {
    removeClassesOnFocus(false);
    addClass(document.getElementById('message'), 'dropzone-wrap');
    addClass(document.getElementById('dragNdrop'), 'dropzone-content');
}

function removeClassesOnFocus(evt) {
    // evt.target is the element that triggered the event. We bound to #message.
    var message = (evt === false) ? document.getElementById('message') : evt.target;
    removeClass(message, 'dropzone-wrap');
    removeClass(message.parentNode, 'dropzone-content');
}

The full converted code:

// http://youmightnotneedjquery.com/#add_class
function addClass(el, className) {
    if (el.classList) { el.classList.add(className); }
    else { el.className += ' ' + className; }
}

// http://youmightnotneedjquery.com/#remove_class
function removeClass(el, className) {
    if (el.classList) { el.classList.remove(className); }
    else { el.className = el.className.replace(new RegExp('(^|\\b)' + className.split(' ').join('|') + '(\\b|$)', 'gi'), ' '); }
}

// When the txtDrop toggle is clicked, add these classes to the message and drag/drop thing.
function txtDropClicked(evt) {
    removeClassesOnFocus(false);
    addClass(document.getElementById('message'), 'dropzone-wrap');
    addClass(document.getElementById('dragNdrop'), 'dropzone-content');
}

// Removes the classes after the message receives focus.
function removeClassesOnFocus(evt) {
    // evt.target is the element that triggered the event. We bound to #message.
    var message = (evt === false) ? document.getElementById('message') : evt.target;
    removeClass(message, 'dropzone-wrap');
    removeClass(message.parentNode, 'dropzone-content');
}

// Attach the click handler to txtDrop toggle when the page loads.
window.addEventListener('load', function() {
    document.getElementById("ToggleTxTDrop").addEventListener( 'click' , txtDropClicked );
    document.getElementById('message').addEventListener('focus', removeClassesOnFocus);
});

It is worth noting that you can use inline functions as handlers, too - and not define txtDropClicked or removeClassesOnFocus. Either way, the point of each method is to perform a single task, and to do that in the most reusable manner possible.

\$\endgroup\$
  • \$\begingroup\$ Terrific! That's some great learning there. I have this code in place and it works perfectly now. Could you tell me more about the dom element? Would this be: "txtDropClicked" the dom element? \$\endgroup\$ – user2513846 Jun 13 '15 at 23:17
  • \$\begingroup\$ This code is just a reworking of what you already had, and assumed the same markup. The DOM element IDs are what you had previously defined in getElementById. txtDropClicked() is an event handler which is being bound to #ToggleTxTDrop. \$\endgroup\$ – Ben Brock Jun 18 '15 at 17:03
0
\$\begingroup\$

I am puzzled by the lack of parallelism.

  • Your changeClass() would be more accurately called addClasses(). However, the inverse operation is just an anonymous function for $('#message').on(…).
  • $('#message').on(…) uses jQuery, but nothing else does.
  • $('#message').on(…) refers to $(this).parent() which presumably refers to the same element as document.getElementById("dragNdrop").

In addition, you have one onClick handler and one onFocus handler. I don't know if you are intentionally listening for two different types of events.

Once you clean up all of these inconsistencies, the code should be a lot easier to understand.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.