2
\$\begingroup\$

I was hoping people could help me review the following page that I've been working on. The page has a simple form that looks at a list of colours and draw a few divs with those colours as backgrounds (so I could see what the colours look like). I'm hoping to build a few complex interactions on this page, but I feel this will quickly grow to be more complex and was hoping to get some pointers.

I know I can split up the CSS and JS sections out in their own files, however, I was hoping for more detailed info:

  • Quick wins with bootstrap that I could leverage, instead of the manual jQuery tweaking that I'm doing
  • CSS conventions I should follow
  • Any other libraries that might help me
  • I'm sure I'm missing a lot, and hope to gain some insights from replies here.

You can also play with the code here.

<html>
<head>
<script src="http://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<script src="http://code.jquery.com/jquery-1.10.2.js"></script>
<script src="http://code.jquery.com/ui/1.11.2/jquery-ui.js"></script>
<script src="//maxcdn.bootstrapcdn.com/bootstrap/3.3.1/js/bootstrap.min.js"></script>
<link href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.1/css/bootstrap.min.css" rel="stylesheet">
<style>
  .colour-box-frame {
    margin: 1px;
    text-align: center;
    margin-left: auto;
    margin-right: auto;
  }
  .colour-box-img {
    border: 1px solid #666;
    padding: 50px;
  }
  .colour-box-text {
    font-family: "inherit";
    font-size: 20px;
    border: 1px solid #666;
  }
  #colour-csv {
    width: 90%;
  }
  .colour-input-div {
    margin: 10px 5px 5px 5px;
  }
  .tab-options {
    cursor: pointer;
  }
</style>
</head>
<body>
  <div class="container">
    <div class="jumbotron">
      <h1>Playing with Colours</h1>

      <ul class="nav nav-tabs">
        <li role="presentation" class="tab-options active" id="raw-colour-list"><a>Raw Colour list</a></li>
        <li role="presentation" class="tab-options" id="html-parse"><a>HTML Parse</a></li>
      </ul>

      <div id="content-placeholder">
        <div id="content-raw-colour-list">
          <div class="colour-input-div"><input id="colour-csv"></input></div>
          <div class="colour-input-div"><strong>Sample Input: </strong>#ffffff; #000; #555d50; #474747; #929292; #393939; #bada55;</div>
          <div class="colour-input-div"><button class="btn btn-lg btn-primary" id="colour-process-button">Process</button></div>
        </div>
        <div id="content-html-parse" class="hidden">
          <div class="colour-input-div"><textarea rows="9" cols="100" placeholder="Paste the source code in here .. We'll fetch all the colours for you"></textarea></div>
          <div class="colour-input-div"><button class="btn btn-lg btn-primary" id="source-process-button">Process</button></div>
        </div>
      </div>

    </div>
    <div id="canvas"></div>
  </div>

  <div id="colour-box-template" class="colour-box-frame col-md-2 hidden">
    <div class="colour-box-img"></div>
    <div class="colour-box-text"></div>
  </div>

</body>

<script>
  $( document ).ready(function() {
    $("#html-parse").click(function (e) {
      e.preventDefault();
      $(".tab-options").each(function( index ) {
        $(this).removeClass('active');
      });
      $(this).addClass('active');
      $('#content-raw-colour-list').addClass('hidden');
      $('#content-html-parse').removeClass('hidden');
    });
    $("#raw-colour-list").click(function (e) {
      e.preventDefault();
      $(".tab-options").each(function( index ) {
        $(this).removeClass('active');
      });
      $(this).addClass('active');
      $('#content-raw-colour-list').removeClass('hidden');
      $('#content-html-parse').addClass('hidden');
    });

    $( "#canvas" ).sortable();
    $( "#canvas" ).disableSelection();

    $("#colour-process-button").click(function(event){
      event.preventDefault();
      var colours = $("#colour-csv").val().split(";");
      var colourHtml = '';
      for (index in colours) {
        var colour = colours[index].replace('#', '').trim();
        if (colour == "") continue;
        if (colour.length == 3) colour = colour + colour;
        var template = $("#colour-box-template").clone().removeClass('hidden');
        template.attr("id", "#colour-box-template_" + index);
        template.children(".colour-box-text").text(colour.toUpperCase());
        template.children(".colour-box-img").css("background-color", colour);
        $('#canvas').append(template);
      }
    });
  });
</script>
</html>
\$\endgroup\$
  • \$\begingroup\$ The demo is not adding background-color to the output when I try it. Does it work for you? \$\endgroup\$ – kleinfreund Dec 25 '14 at 11:16
  • \$\begingroup\$ For some reason its' not working in the jsfiddle link I pasted above, but if you copy / paste the code I have in htmledit.squarefree.com, you should see it work. \$\endgroup\$ – Prashant Dec 25 '14 at 17:54
5
\$\begingroup\$

Loading jQuery twice

You're loading both jquery.min.js and jquery-1.10.2.js. I assume that that is an unintentional mistake.

Use of jQuery

jQuery is called jQuery because you can use it to operate on sets of elements that you specify using a selector. When you set the onclick handlers using two calls with two selectors $('#html-parse') and $('raw-colour-list'), you aren't using jQuery effectively.

jQuery calls are also chainable, so you can write $('#canvas').sortable().disableSelection();.

template.attr("id", "#colour-box-template_" + index) is wrong — and altogether superfluous. It's wrong because the ID must begin with a letter. Also, since index is localized to the function, you'll end up assigning duplicate IDs on subsequent runs of the onclick handler.

Playing with colours

It's one thing to play with colours in your browser; playing with the colour specification in your code is not something I would recommend, because:

  • You stripped off the # from the colour specification, but neglected to put it back. Therefore, the whole demo is completely broken (in Firefox and Safari, at least).
  • You messed up interpreting #abc as #abcabc, when it should be #aabbcc.
  • It's not 1998 anymore. There is a CSS Color Module Level 3 recommendation, which is much more complex than #012345. For example, you should be able to specify 'red' or rgb(255, 0, 0) instead of #f00.
  • The point of this kind of demo, in my view, is to see how your browser behaves. For example, there is CSS Color Module Level 4 draft specification — does your browser support the new features? If the JavaScript code munges the colour specification, then you can't reliably tell anymore what is going on.

Therefore, I recommend that you pre-process the colour specification string as little as possible — perhaps just trimming whitespace.

Incidentally, I recommend writing all code using American English, as that is the international norm for code. British spellings just make the code more confusing to maintain, and the cognitive dissonance between names like background-color and your colour variable are annoying to me. (Use whatever language you want when writing comments.)

<html>
<head>
<script src="//code.jquery.com/jquery-1.10.2.js"></script>
<script src="//code.jquery.com/ui/1.11.2/jquery-ui.js"></script>
<script src="//maxcdn.bootstrapcdn.com/bootstrap/3.3.1/js/bootstrap.min.js"></script>
<link href="//maxcdn.bootstrapcdn.com/bootstrap/3.3.1/css/bootstrap.min.css" rel="stylesheet">
<style>
  .colour-box-frame {
    margin: 1px;
    text-align: center;
    margin-left: auto;
    margin-right: auto;
  }
  .colour-box-img {
    border: 1px solid #666;
    padding: 50px;
  }
  .colour-box-text {
    font-family: "inherit";
    font-size: 20px;
    border: 1px solid #666;
  }
  #colour-csv {
    width: 90%;
  }
  .colour-input-div {
    margin: 10px 5px 5px 5px;
  }
  .tab-options {
    cursor: pointer;
  }
</style>
</head>
<body>
  <div class="container">
    <div class="jumbotron">
      <h1>Playing with Colours</h1>

      <ul class="nav nav-tabs">
        <li role="presentation" class="tab-options active" id="raw-colour-list"><a>Raw Colour list</a></li>
        <li role="presentation" class="tab-options" id="html-parse"><a>HTML Parse</a></li>
      </ul>

      <div id="content-placeholder">
        <div id="content-raw-colour-list">
          <div class="colour-input-div"><input id="colour-csv"></input></div>
          <div class="colour-input-div"><strong>Sample Input: </strong>#ffffff; #000; #555d50; #474747; #929292; #393939; #bada55;</div>
          <div class="colour-input-div"><button class="btn btn-lg btn-primary" id="colour-process-button">Process</button></div>
        </div>
        <div id="content-html-parse" class="hidden">
          <div class="colour-input-div"><textarea rows="9" cols="100" placeholder="Paste the source code in here .. We'll fetch all the colours for you"></textarea></div>
          <div class="colour-input-div"><button class="btn btn-lg btn-primary" id="source-process-button">Process</button></div>
        </div>
      </div>

    </div>
    <div id="canvas"></div>
  </div>

  <div id="colour-box-template" class="colour-box-frame col-md-2 hidden">
    <div class="colour-box-img"></div>
    <div class="colour-box-text"></div>
  </div>

</body>

<script>
  $( document ).ready(function() {
    $(".tab-options").click(function (e) {
      e.preventDefault();
      $(".tab-options").removeClass('active');
      $(this).addClass('active');
      $('#content-placeholder').children().addClass('hidden');
      $('#content-' + $(this).attr('id')).removeClass('hidden');
    });

    $("#canvas").sortable().disableSelection();

    $("#colour-process-button").click(function(event){
      event.preventDefault();
      var colours = $("#colour-csv").val().split(";");
      for (index in colours) {
        var colour = colours[index].trim();
        if (colour === '') continue;
        var template = $("#colour-box-template").clone().removeClass('hidden');
        template.children(".colour-box-text").text(colour.toUpperCase());
        template.children(".colour-box-img").css("background-color", colour);
        $('#canvas').append(template);
      }
    });
  });
</script>
</html>

\$\endgroup\$
  • \$\begingroup\$ Thank you for such a detailed answer. I made some of the changes you suggested and am trying to understand the other before I get to change my code. \$\endgroup\$ – Prashant Dec 25 '14 at 18:01
1
\$\begingroup\$

Bootstrap

I have nothing nice to say about Bootstrap. Such a simple demo and you're asking the user to download all of that? Even worse, your markup is littered with unsemantic class names. It might be faster while prototyping, but you're going to pay for it down the road when it comes time to do the real thing.

Incorrect CSS

In .colour-box-text, you've specified font-family: "inherit". Unless there is a font somewhere named Inherit, this isn't doing what you think it's doing. The inherit keyword needs to be unquoted. You shouldn't even need this anyway.

\$\endgroup\$
  • \$\begingroup\$ I'd love to better understand why bootstrap is such a bad option in your opinion (both in general and for this project). I'm hoping to build on this project, the current state is no where near being finished but I wanted to share an initial skeleton to get feedback on some shortcomings that I could avoid. re: "asking the user to download all of that" .. I'm pointing to a public cdn so chances might be high the user already has it. Is there a better way? OR a better library you would recommend? Also: thanks for the font-family comment, it was not what I wanted :) \$\endgroup\$ – Prashant Dec 25 '14 at 18:11
  • 1
    \$\begingroup\$ I have not paid attention to BS3, but the problem with themes like BS is that they are too general and tend to be loaded with unsemantic class names. If it weren't for the fact that you are using BS and it's designed to be used that way, I would have been more insistent that you not do that. Every time you use class names that describe what your elements look like, you pay for it down the line when it comes time to redesign. So no, I could not recommend any that I have ever seen. For my own work, I rolled my own modularized Sass toolbox/library. \$\endgroup\$ – cimmanon Dec 26 '14 at 15:08
  • 1
    \$\begingroup\$ Loving the abbreviation for Bootstrap there \$\endgroup\$ – gengkev Jan 2 '15 at 17:04
1
\$\begingroup\$

The other answers already cover the issues with your code quite well.

I'm going to add something about the way you present your sample input. It's only somewhat helpful. For example, what exactly is the ; doing? Is it delimiting the values so one can enter multiple values at once? Why does the last value in your sample input have it than?

I'd recommend explaining that and show some one or two extra examples (e.g. for only 1 value).

\$\endgroup\$
  • \$\begingroup\$ Yea .. That does feel weird. It is suppose to act as a delimiter, but I think just having space separated values might be more intuitive. \$\endgroup\$ – Prashant Dec 25 '14 at 18:13

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.