2
\$\begingroup\$

I've never had anyone review my code. I actually found full-time work as a web developer and in the interest of ensuring that I'm always improving, I'd like to see if I'm doing things the proper way. I accept that some aspects are semantics, i.e. two or four space indentation.

/**
 * Name:        jQuery Tab Scroller
 * Description: A tab scroller using jQuery
 * @package     Chimera Apps
 * @version     1.0.3
 * @author      Chimera.Zen
 * @copyright   Copyright (c) 2017, Chimera.Zen
 * @link        https://github.com/ChimeraZen
 * @license     http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
 */

var scroll_distance = 275,    // Distance .tab-container should scroll when <i> is clicked
    animate_speed   = 400;    // Speed at which .tab-container should animate the scroll

/** Load the scroller details **/
function get_scroll_details(scroller) {
  "use strict";
  var tab_true_width  = Math.round(scroller.siblings('.tab-container').children('ul').width()),
      container_width = Math.round(scroller.siblings('.tab-container').width()),
      left_scrolled   = Math.round(scroller.siblings('.tab-container').scrollLeft()),
      scrolls = {
        "distance" : tab_true_width - container_width,
        "leftscrolled" : left_scrolled,
        "remaining" : tab_true_width - container_width - left_scrolled,
        "scroll_next" : scroller.parent().children('.scroller.next'),
        "scroll_prev" : scroller.parent().children('.scroller.prev')
      };
  return scrolls;
}

/** Tab Scroller **/
function tab_switch(scroller) {
  "use strict";
  var scrolls = get_scroll_details(scroller);
  if (scrolls.leftscrolled === 0) {
    scrolls.scroll_next.css("visibility", "visible");
    scrolls.scroll_prev.css("visibility", "hidden");
  } else if (scrolls.remaining === 0) {
    scrolls.scroll_next.css("visibility", "hidden");
    scrolls.scroll_prev.css("visibility", "visible");
  } else {
    scrolls.scroll_next.css("visibility", "visible");
    scrolls.scroll_prev.css("visibility", "visible");
  }
}

/** Animate and check if <i> visibility needs to switch **/
function scroll_it(scroller, scroll) {
  "use strict";
  scroller.siblings('.tab-container').animate({scrollLeft: scroll}, animate_speed, function () {
    tab_switch(scroller);
  });
}

/** Animate & Scroll on Click **/
$('.scroller.next').click(function () {
  "use strict";
  var scroller  = $(this),
      scrolls   = get_scroll_details(scroller);
  if (scrolls.remaining >= scroll_distance) {
    scroll_it(scroller, scrolls.leftscrolled + scroll_distance);
  } else {
    scroll_it(scroller, scrolls.leftscrolled + scrolls.remaining);
  }
});

$('.scroller.prev').click(function () {
  "use strict";
  var scroller  = $(this),
      scrolls   = get_scroll_details(scroller);
  if (scrolls.leftscrolled !== 0) {
    scroll_it(scroller, scrolls.leftscrolled - scroll_distance);
  } else {
    scroll_it(scroller, 0);
  }
});
.tab-scroller{
  display:flex;
  justify-content:space-around;
  align-items:center;
  width:100%;
  max-height:50px;
}
.tab-container{
  max-width:50%;
  overflow:hidden;
}
.tab-scroller i {
  padding: 15px;
  font-size:20px;
  cursor:pointer;
}
.tab-scroller .prev{
  visibility:hidden;
}
.tab-scroller .tab-container ul{
  display:flex;
  width: max-content;
  padding: 0;
}
.tab-scroller .tab-container li{
  display:flex;
  margin-right:10px;
}
.tab-scroller .tab-container li:last-child{
  margin-right:0;
}
<html>
  <head>
    <link type="text/css" src="tab-scroller.css" />
    <script type="text/javascript" src="tab-scroller.js"></script>
  </head>
  <body>
    <div class="tab-scroller">
      <!-- <i class="scroller prev">&lt;</i> -->
      <i class="fa fa-chevron-left scroller prev"></i>
      <div class="tab-container">
        <ul>
          <li>Archery</li>
          <li>Baseball</li>
          <li>Basketball</li>
          <li>Boxing</li>
          <li>Football</li>
          <li>Golf</li>
          <li>Soccer</li>
          <li>Surfing</li>
        </ul>
      </div>
      <!-- <i class="scroller prev">&gt;</i> -->
      <i class="fa fa-chevron-right scroller next"></i>
    </div>
  </body>
</html>

I made a working example in JSFiddle and I also work in PHP, making alterations to Joomla and Wordpress plugins, but my PHP scripts are written the same way as my JS. If you've read this far and are willing to help, I don't want to waste your time if I can help it!

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 13 '17 at 20:45
4
\$\begingroup\$

This looks pretty good to me, is there a reason you have "use strict" inside every function in your js, but not in the beginning of the script instead?

You might have a reason not to, but if this is not the case you can benefit from adapting your code to some ES6 features, such as const, let and arrow functions.

Another thing I noticed, in your case you have only one scroller on each side so your event listeners can be modified from

$('.scroller.prev').click(function () {
  "use strict";
  var scroller  = $(this),
  // more code
});

into

var scroller_prev = $('.scroller.prev');
scroller_prev.click(function () {
  "use strict";
  var scroller  = scroller_prev,
  // more code
});

This way you do not have to recall jquery function to create a jq object which you already did before, caching these object is a good practice when working with jquery. However, this might not be the result you are after if you have more than one previous scroller and you only want to update the one that was clicked.

I would be nitpicking, but it is a fare mention - consistency in naming conventions, in javascript the most used naming is camel case ie myVariableName. Which might be a preference thing, I can see that you used underscore case ie tab_true_width but not for all leftscrolled. It is up to you which convention to use, but once you pick one you should stick to it.

Overall this code can be understood fairly easily, good job writing it and sharing here with a jsFiddle too!

\$\endgroup\$
  • \$\begingroup\$ Thank you @AntK :) The only reason I use "use strict" so often is because jslint complained if it wasn't in every function. I'm definitely going to look into ES6 features! You're correct again :D I did design this so multiple tabs could be run on the same page. There was something that was itching at me and I just couldn't put my finger on it. Ahh leftscrolled...a extra thank you for that one! lol I just figured out shift + enter lol THANK YOU AGAIN! p.s. I just realized shift + enter does nothing in a comment lol \$\endgroup\$ – Chimera.Zen Dec 13 '17 at 1:46
  • \$\begingroup\$ np, please note that you do not have to write "use strict" in every single function, but only on top of your script. you can read up on this more here, as for eslint, you can edit the eslintrc file to tune it to your preference. Cheers \$\endgroup\$ – AntK Dec 13 '17 at 2:00
4
\$\begingroup\$

I agree with the advice of AntK - especially about use strict only required once.

Another aspect to consider is that when applying CSS using .css() will alter the style attribute directly, which will cause a repaint and reflow1.

Instead of modifying the style attributes directly, add and remove classes. For example, the following CSS classes can be added:

.visible {
  visibility: visible;
}
.hidden {
  visibility: hidden;
}

And remove the visibility: hidden on .tab-scroller .prev. Then the logic in tab_switch() can be simplified using .toggleClass():

var nextVisible = scrolls.leftscrolled === 0 || scrolls.remaining !== 0;
var prevVisible = scrolls.remaining === 0 || scrolls.leftscrolled !== 0;
scrolls.scroll_next.toggleClass('visible', nextVisible).toggleClass('hidden', !nextVisible);
scrolls.scroll_prev.toggleClass('visible', prevVisible).toggleClass('hidden', !prevVisible);

See an updated fiddle here.

Update

I guess altering the class instead of the style still causes a reflow, but it should hopefully keep the display logic (i.e. CSS) out of the javascript...


1https://ilikekillnerds.com/2015/02/stop-writing-slow-javascript/ - section Don't touch my DOM bro

\$\endgroup\$
  • \$\begingroup\$ Thanks! A side question for you Sam. Is it considered bad form to directly alter the style attribute, or more to the point, is it wrong to use the style attribute? I seem to recall reading that somewhere. \$\endgroup\$ – Chimera.Zen Dec 13 '17 at 19:04
  • \$\begingroup\$ See the cited article in the section under the heading Don't touch my DOM bro (I wish there were anchors on that page I could use to send you to that section), which discusses benefits of altering style via class names instead of style attribute. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 13 '17 at 19:06
  • 1
    \$\begingroup\$ toggleClass does not trigger reflow itself, the styles properties you change is what determines if a reflow going to happen or not, csstriggers is a good resource to check it, in this case with toggling visibility reflow will happen regardless. Yet, I agree that having the styles in the css and logic in the js is whole lot easier to understand. \$\endgroup\$ – AntK Dec 14 '17 at 12:30

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.