current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I am writing a Q&A wap page and want to switch questions in one page. This is my first time to write JS and CSS code, so I think there must be some improvement.

$(function(){
    $("#parent").on('click','.children', function(e){
        var that = e.currentTarget;
        var wid = parseInt($(that).parent().css('width'));
        var margin = parseInt($(that).parent().css('marginLeft'));
        margin -= wid;
        $(that).parent().css('marginLeft', margin + "px");
    });
});
#wrap {
  width: 100%;
  height: 30px;
  overflow: hidden;
}
#parent {
  width: 100%;
  -webkit-transition: all .2s linear;
  white-space: nowrap;
}
#parent div {
  background-color: gray;
  width: 100%;
  height: 30px;
  display: inline-block;
}
<!DOCTYPE html>
<html>
<head lang="en">
    <meta charset="UTF-8">
    <title></title>
    <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, minimum-scale=1.0, user-scalable=no" />
    <meta name="apple-touch-fullscreen" content="yes" />
    <meta name="apple-mobile-web-app-capable" content="yes" />
    <meta name="apple-mobile-web-app-status-bar-style" content="black" />
    <meta name="format-detection" content="telephone=no" />

    <link rel="stylesheet" href="../css/change.css"/>
    <script src="http://code.jquery.com/jquery-2.1.3.min.js"></script>
</head>
<body>

<div id="wrap">
    <div id="parent">
        <div class="children">NO.1</div><div class="children">NO.2</div><div class="children">NO.3</div><div class="children">NO.4</div><div>result</div>
    </div>
</div>

<script src="../js/change.js"></script>
</body>
</html>

Running example

I don't want to my last result div to switch, so there is no event listener on my last result div.

share|improve this question
    
One thing I'd do is to define a variable for that.parent() call. That way your code would be more readable and you might even gain some performance –  Daniel Sokolov Apr 28 at 10:31

3 Answers 3

up vote 2 down vote accepted

JavaScript

I would make the following changes in the jQuery code:

1) Create a private scope for your code using an IIFE.
2) Cache your selectors.
3) Supply the "radix" parameter to parseInt.

(function( $ ) { //start IIFE to create a private scope and $=jQuery

  function updateElement() {
    var $this = $(e.currentTarget),  //cache selectors
        $parent = $this.parent(),
        wid = 0, margin = 0;
    wid = parseInt( $parent.css('width'), 10); //added radix parameter
    margin = parseInt( $parent.css('marginLeft'), 10);
    $parent.css('marginLeft', (margin - wid) + "px");
  }

  $(function(){
    $("#parent").on( 'click', '.children', updateElement ); 
  });

})( jQuery );  //pass jQuery to function so $ is always set correctly.

If you are pointing to the #parent element you could change the JS to be more performant by caching the parent element as well.

 (function( $ ) { 

  var $parent = $('#parent');

  function updateElement() {
    var wid = 0, margin = 0;
    wid = parseInt( $parent.css('width'), 10); //added radix parameter
    margin = parseInt( $parent.css('marginLeft'), 10);
    $parent.css('marginLeft', (margin - wid)+ "px");
  }

  $(function(){
    $parent.on( 'click', '.children', updateElement ); 
  });


 })(jQuery);

CSS

In your CSS you should remove the vendor prefixes or supply all of them ( Here is a list. Ok not all just the most common: ms,o,moz,webkit,etc ). Also, you should include the non-prefixed property last in the list so that once it is available non-prefixed your code still works.

#parent {
  -moz-transition: all .2s linear;
  -o-transition: all .2s linear;
  -ms-transition: all .2s linear;
  -webkit-transition: all .2s linear;
  transition: all .2s linear;
}

You could also use a tool like autoprefixer in your build process to add the prefixes automatically. I would recommend this approach.

share|improve this answer
    
I have learnt a lot from it. thanks :) –  Felix Apr 29 at 2:34
up vote 1 down vote

Personally I would wrap your text in a p tag and then set an id for each part of the text. That way you can manipulate each of your questions' text directly without having to worry about moving the divs and you can use the divs to place the the switched texts.

If you are worried that will throw off styling, perhaps make a class for each of the paragraphs to make them styled as if they weren't regular paragraphs and apply that as well.

share|improve this answer
up vote 1 down vote

Allow me to add a few remarks on your HTML and css.

About your markup:

  • It is quite obvious to me that that list of questions should in fact be a ul.
  • Be careful with id's. Only use an id when you are absolutely sure there will only be one of them on your page. Things like wrap and parent are not names I would use for an id, as it is quite likely there well be more of those on your page down the line.
  • You do not have to add a class or id to every element you want to be able to access trough css or js. You can use selectors that are slightly more complex. They reduce the length of your code and will make it much easier to maintain your page (what if you add an item and accidentally type `childern', those are horrible bugs to track down). I admit that this will be slightly less performant, but I doubt you will be able to see or even measure the difference. And in my book maintainability and readability is more important than performance.

That being said, my markup would look something like this:

<div id="questions-wrapper">
    <ul>
        <li>NO.1</li>
        <li>NO.2</li>
        <li>NO.3</li>
        <li>NO.4</li>
        <li>result</li>
    </div>
</div>

When it comes to your css, most of it has been said by others, but I would like to warn you about using all for transition. That's like firing a bazooka at a fly. Absolutely no need for it, and always slower than specifying the exact property you want to transition. You probably won't see the difference in the page you have now, but it will become noticeable once the page becomes bigger and more complex.

One last thing, you are now transition the margin-left property. This is quite costly for the browser, and should be avoided whenever possible. There are a few properties that are much cheaper to transition. In your case it would be much better to work with a transform: translateX(...);. Have a look at this blogpost for an explanation on the how and why. And note that the difference is actually pretty huge, it can easily double the framerate of your animation.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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