5
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$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\$\endgroup\$
    – Daniel Sokolov
    CommentedApr 28, 2015 at 10:31

3 Answers 3

Reset to default
3
\$\begingroup\$

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.

\$\endgroup\$
0
    2
    \$\begingroup\$

    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.

    \$\endgroup\$
      1
      \$\begingroup\$

      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.

      \$\endgroup\$

        Start asking to get answers

        Find the answer to your question by asking.

        Ask question

        Explore related questions

        See similar questions with these tags.