5
\$\begingroup\$

I've been coding for a while but I wouldn't really consider myself all that good at it. I recently started to learn bootstrap, and I really liked it and made a page to test it out. It has some basic bootstrap layouts and a little bit of jQuery.

I would like to learn better and more efficient ways to code, so any advice on better formatting and code would be very appreciated. Some spacing may be a bit weird because of formatting. Here is an example in Codepen.

$(document).ready(function(){
$('[data-toggle="tooltip"]').tooltip();
});
//button 1
$(document).ready(function(){
$("#button1").click(function(){
    $("#text1").toggle();
});
});
//button 2
$(document).ready(function(){
$("#button2").click(function(){
    $("#text2").fadeToggle();
});
});
//button 3
$(document).ready(function(){
$("#button3").click(function(){
                
   
//Had to break this line into 2 so I could post this, wouldn't let me indent/space right
$("#text3").fadeIn().slideUp(500).slideDown(500)
.slideUp(500).slideDown(500).fadeOut();
    });
});
 .div1 {
  background-color:#80bfff;
}

.div2 {
  background-color:#ccffff;
  text-overflow: scroll;
}

.div3 {
  background-color:#80bfff;
}

.row {
  height: calc(100vh - 300px);
}

#main {
  background-color: #3333ff;
}

#main_head {
  height: 150px;
  margin: 0;
}

#main_foot {
  height: 150px;
  background-color:#3333ff;
  margin: 0;
 position: absolute;
 left: 0;
 right: 0;

  }
  
  h1 {
    color: black;
  }
  
  .drop1 {
    background-color: #66ff99;
  }
  
  #coll_button {
    height: 50px;
    width: 200px;
    border-radius: 6px;
    margin-top: 30px;
    margin-left: 20px;
  }
  
  #text1 {
    padding-left: 10px;
    padding-top: 10px;
    display:none;
  }
  
  #text2 {
    padding-left: 10px;
    padding-top: 10px;
    display:none;
  }
  
  #text3 {
    padding-left: 10px;
    padding-top: 10px;
    display:none;
  }
  
  #modalbutton1 {
    height: 50px;
    margin-left: 20px;
    margin-top: 20px;
  }
}
<!DOCTYPE html>
<html>
  <html lang="en">
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width, initial-scale=1">
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-beta.2/css/bootstrap.min.css">
<script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.12.6/umd/popper.min.js">    </script>
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-beta.2/js/bootstrap.min.js"></script>

</head>
<body>
  
<link rel="stylesheet" href="test.css">
<script src="test.js"></script>
<div class="container-fluid" id="main">
  <h1 id="main_head">This is a heading</h1>
  
  <div class="row">
    
    <div class="col-sm-2 div1">
      <div class="btn-group-vertical">
        <h4>These buttons don't work yet because I haven't implemented anything yet</h4>
        <button id="button1" type="button" class="btn btn-warning">Toggle text</button>
        <button id="button2" type="button" class="btn btn-info">Fade text</button>
        <button id="button3" type="button" class="btn btn-success">Text animation</button>
      </div>
    </div>
    
    <div class="col-sm-8 div2">
      <button data-toggle="collapse" data-target="#main_collapse" class="btn-danger" id="coll_button">Collapsible</button>
      
      <div id="main_collapse" class="collapse">
        <h3>Some pretty neat random text that just appears when you click on the collapse thing</h3>
      </div>
      <br>
      <!--Start modal-->
      <!-- Trigger the modal with a button -->
      <button type="button" class="btn btn-info btn-lg" id="modalbutton1" data-toggle="modal" data-target="#myModal">Open Modal</button>

      <!-- Modal -->
      <div id="myModal" class="modal fade" role="dialog">
        <div class="modal-dialog">

          <!-- Modal content-->
          <div class="modal-content">
            <div class="modal-header">
              <h4 class="modal-title">Modal Header</h4>
              <button type="button" class="close" data-dismiss="modal">&times;</button>
            </div>
            <div class="modal-body">
              <p>This is a modal. It's a cool way of displaying pop up text at the click of a button.</p>
            </div>
            <div class="modal-footer">
              <button type="button" class="btn btn-success" data-dismiss="modal">Close</button>
            </div>
          </div>

        </div>
      </div>
      <!--End modal-->
      
      <h4 id="text1">This is some toggleable text</h4>
      <h4 id="text2">You can fade this text in and out</h4>
      <h4 id="text3">You can make this text slide up and down</h4>
    </div>
    
    <div class="col-sm-2 div3">
      <div class="container">
        <h4>This is a dropdown menu</h4>
        <div class="dropdown">
        <button class="btn btn-success dropdown-toggle" type="button" data-toggle="dropdown">Click the dropdown menu
        <span class="caret"></span></button>
          <ul class="dropdown-menu drop1">
            <li class="drop1"><a href="#">Dropdown 1</a></li>
            <li class="drop1"><a href="#">Dropdown 2</a></li>
            <li class="drop1"><a href="#">Dropdown 3</a></li>
          </ul>
        </div>
      </div>
    </div>
    
  </div>
  
  <div id="main_foot" class="container-fluid"><h1>This is a footer</h1>                
</div>
</div>
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.5/js/bootstrap.min.js"></script>
</body>
</html>

|improve this question|||||
\$\endgroup\$
3
\$\begingroup\$

Naming

You should start naming your classes something a little more readable than div1 as all that tells me that it's a div but nothing regarding functionality. Like, instead of div1 I'd use something like primary-content as my class name.

Document.ready

Next, you should wrap all your functions inside of one $(document).ready(). It increases readability and does the exact same thing.

$(document).ready(function(){  
  //button 1
  $("#button1").click(function(){
      $("#text1").toggle();
  });

  //button 2
  $("#button2").click(function(){
      $("#text2").fadeToggle();
  });
  //button 3
    $("#button3").click(function(){
      //Had to break this line into 2 so I could post this, wouldn't let me indent/space right
      $("#text3")
        .fadeIn()
        .slideUp(500)
        .slideDown(500)
        .slideUp(500)
        .slideDown(500)
        .fadeOut();
    });
});

Indentation and trailing } and });

Your indentation seems to be a little off. This is a common thing that happens. It is easy to fix (unless there is more than just indentation problems). I frequently Google <language name> beautifier. What a beautifier does is indent your code with inhuman precision. Try a CSS beautifier. I say this because beautifying/indenting correctly can make debugging easier. I found a couple of extra tidbits that should not be there }.

CSS Selectors

It is also considered good practice to make your CSS selectors as precise as possible (without impeding your program), like instead of .primary-content, you could use div.container > div.primary-content. Where the container contains primary content.

Having imprecise CSS selectors can backfire. For instance, when you go to use h1 somewhere else and the page and it ends up being blue because you set h1{color:blue}. Of course, you can override that styling, but you may get a little sick of doing that after you have to manually set your x-th h1's color back to default.

You can also massively reduce repetitive class setting by using parent selectors:

.drop1 > li, drop1 {
  background-color: #66ff99;
  border-color:#66ff99;
}

Goes from:

<ul class="dropdown-menu drop1">
  <li class="drop1"><a href="#">Dropdown 1</a></li>
  <li class="drop1"><a href="#">Dropdown 2</a></li>
  <li class="drop1"><a href="#">Dropdown 3</a></li>
</ul>

To:

<ul class="dropdown-menu drop1">
   <li><a href="#">Dropdown 1</a></li>
   <li><a href="#">Dropdown 2</a></li>
   <li><a href="#">Dropdown 3</a></li>
</ul>

Grouping selectors can also reduce code (making it easier to error check and find what you're looking for and update all relevant elements):

This:

#text1 {
  padding-left: 10px;
  padding-top: 10px;
  display: none;
}

#text2 {
  padding-left: 10px;
  padding-top: 10px;
  display: none;
}

#text3 {
  padding-left: 10px;
  padding-top: 10px;
  display: none;
}

Should be:

#text1, #text2, #text3 {
  padding-left: 10px;
  padding-top: 10px;
  display: none;
}

But even then you could further reduce it by using padding: <top>px <right>px <bottom>px <left>px; or, for your code:

#text1, #text2, #text3 {
  padding: 10px 0 0 10px;
  display: none;
}

Button formatting

I'd personally try to steer away from manually setting width and height in the case of a button because if you were to change the text at all in the future, it will not display as intended. There are cases where it's okay to set button size to the pixel but that is on a case-by-case basis. So, I'd instead use padding for what you're doing.

#coll_button {  
  border-radius: 6px;
  padding:10px 40px 10px 40px;
  margin-top: 30px;
  margin-left: 20px;
}

jQuery indentation

When I am chaining jQuery functions, I commonly use another indent for each function call. I find it to be more readable.

$("#button3").click(function(){
  $("#text3")
    .fadeIn()
    .slideUp(500)
    .slideDown(500)
    .slideUp(500)
    .slideDown(500)
    .fadeOut();
});

Disclaimer

I changed the height of your row to 200% because I wanted to test how it would look if I included more content.

$(document).ready(function(){  
  //button 1
  $("#button1").click(function(){
      $("#text1").toggle();
  });
  
  //button 2
  $("#button2").click(function(){
      $("#text2").fadeToggle();
  });
  //button 3
    $("#button3").click(function(){
      //Had to break this line into 2 so I could post this, wouldn't let me indent/space right
      $("#text3")
        .fadeIn()
        .slideUp(500)
        .slideDown(500)
        .slideUp(500)
        .slideDown(500)
        .fadeOut();
    });
});
.div1 {
  background-color: #80bfff;
}

body {
  margin:0;
  padding:0;
}

.div2 {
  background-color: #ccffff;
  text-overflow: scroll;
}

.div3 {
  background-color: #80bfff;
}

.row {
  height: 200%;
}

#main {
  background-color: #3333ff;
}

#main_head {
  height: 150px;
  margin: 0;
}

#main_foot {
  height: 150px;
  background-color: #3333ff;
  padding:0;
}

h1 {
  color: black;
}

.drop1 li {
  background-color: #66ff99;
  border-color:#66ff99;
}

#coll_button {  
  border-radius: 6px;
  padding:10px 40px 10px 40px;
  margin-top: 30px;
  margin-left: 20px;
}

.textContainer {
  padding:10px;
}

.textContainer >  h4 {
  display:none;
}

#text1, #text2, #text3 {
  padding: 10px 0 0 10px;
  display: none;
}

#modalbutton1 {
  height: 50px;
  margin-left: 20px;
  margin-top: 20px;
}
<!DOCTYPE html>
<html>
   <html lang="en">
      <head>
         <meta charset="utf-8">
         <meta name="viewport" content="width=device-width, initial-scale=1">
         <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
         <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-beta.2/css/bootstrap.min.css">
         <script src="https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.12.6/umd/popper.min.js">    </script>
         <script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0-beta.2/js/bootstrap.min.js"></script>
      </head>
      <body>
         <link rel="stylesheet" href="test.css">
         <script src="test.js"></script>
         <div class="container-fluid" id="main">
            <h1 id="main_head">This is a heading</h1>
            <div class="row">
               <div class="col-sm-2 div1">
                  <div class="btn-group-vertical">
                     <h4>These buttons don't work yet because I haven't implemented anything yet</h4>
                     <button id="button1" type="button" class="btn btn-warning">Toggle text</button>
                     <button id="button2" type="button" class="btn btn-info">Fade text</button>
                     <button id="button3" type="button" class="btn btn-success">Text animation</button>
                  </div>
               </div>
               <div class="col-sm-8 div2">
                  <button data-toggle="collapse" data-target="#main_collapse" class="btn-danger coll_button" id="coll_button">Collapsible</button>
                  <div id="main_collapse" class="collapse">
                     <h3>Some pretty neat random text that just appears when you click on the collapse thing</h3>
                  </div>
                  <br>
                  <!--Start modal-->
                  <!-- Trigger the modal with a button -->
                  <button type="button" class="btn btn-info btn-lg" id="modalbutton1" data-toggle="modal" data-target="#myModal">Open Modal</button>
                  <!-- Modal -->
                  <div id="myModal" class="modal fade" role="dialog">
                     <div class="modal-dialog">
                        <!-- Modal content-->
                        <div class="modal-content">
                           <div class="modal-header">
                              <h4 class="modal-title">Modal Header</h4>
                              <button type="button" class="close" data-dismiss="modal">&times;</button>
                           </div>
                           <div class="modal-body">
                              <p>This is a modal. It's a cool way of displaying pop up text at the click of a button.</p>
                           </div>
                           <div class="modal-footer">
                              <button type="button" class="btn btn-success" data-dismiss="modal">Close</button>
                           </div>
                        </div>
                     </div>
                  </div>
                  <!--End modal-->
                  <h4 id="text1">This is some toggleable text</h4>
                  <h4 id="text2">You can fade this text in and out</h4>
                  <h4 id="text3">You can make this text slide up and down</h4>
               </div>
               <div class="col-sm-2 div3">
                  <div class="container">
                     <h4>This is a dropdown menu</h4>
                     <div class="dropdown">
                        <button class="btn btn-success dropdown-toggle" type="button" data-toggle="dropdown">Click the dropdown menu
                        <span class="caret"></span></button>
                        <ul class="dropdown-menu drop1">
                           <li><a href="#">Dropdown 1</a></li>
                           <li><a href="#">Dropdown 2</a></li>
                           <li><a href="#">Dropdown 3</a></li>
                        </ul>
                     </div>
                  </div>
               </div>
            </div>
            <div id="main_foot" class="container-fluid">
               <h1>This is a footer</h1>
            </div>
         </div>
         <script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.5/js/bootstrap.min.js"></script>
      </body>
   </html>

|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

Feedback

The diverse colors and UI controls look good- nice work! And good work chaining the animation calls for the slide-up/down text (i.e. #text3).

Suggestions

Javascript/jQuery

Cache DOM references

Instead of looking up the DOM elements every time they are used, they can be stored in a variable and the variable can be referenced. Use const unless Browser compatibility is an issue - then use var.

$(document).ready(function(){
    const text1Element = $("text1");
    $("#button1").click(function(){
        text1Element.toggle();
    });

One can consolidate the DOM-ready callbacks (I.e. $(document).ready(function(){ });) into one, instead of one for each UI initialization call. 

$(document).ready(function(){
    $('[data-toggle="tooltip"]').tooltip();
    //button 1
    $("#button1").click(function(){
        $("#text1").toggle();
    });

    //button 2
    $("#button2").click(function(){
        $("#text2").fadeToggle();
    });

    //button 3
    $("#button3").click(function(){

    //Had to break this line into 2 so I could post this, wouldn't let me indent/space right
    $("#text3").fadeIn().slideUp(500).slideDown(500)
    .slideUp(500).slideDown(500).fadeOut();
});

Event delegate

An event delegate could be used instead of adding a click handler to each button element. That way additional elements could be added without needing to add event listeners to each one- this becomes especially advantageous if elements are added and removed from the DOM throughout the event lifecycle of the page (otherwise memory issues could result if event listeners aren't removed from elements after they are removed from the DOM).

$(document).ready(function() {
  var text1 = $("#text1");
  var text2 = $("#text2");
  var text3 = $("#text3");

  $(this).click(function(clickEvent) {
    switch ($(clickEvent.target).attr("id")) {
      case "button1":
        text1.toggle();
        break;
      case "button2":
        text2.fadeToggle();
        break;
      case "button3":
        text3.fadeIn().slideUp(500).slideDown(500)
            .slideUp(500).slideDown(500).fadeOut();
        break;
    }
  });

DOM-ready callback syntax

And while that syntax of $(document).ready() still works with the latest version (i.e. 3.2.1 at the time of typing), the recommended syntax is simply $(function() {})1.

CSS

The height and margin styles could be removed from the #main_foot selector if that selector is combined with the selector for #main_head:

#main_head,
#main_food {
    height: 150px;
    margin: 0;
}

#main_foot {
    background-color:#3333ff;
    position: absolute;
    left: 0;
    right: 0;
}

The same applies to the text selectors (i.e. #text1, #text2, #text3 could also just use one class for all three)- the styles could be consolidated:

#text1,
#text2,
#text3 {
    padding-left: 10px;
    padding-top: 10px;
    display:none;
}

1http://api.jquery.com/ready/

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ What's the advantage of the event delegate in this context? \$\endgroup\$ – le_m Dec 5 '17 at 20:12
  • 1
    \$\begingroup\$ Thanks for pointing out that I didn't explain benefits... I will update that section \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 5 '17 at 20:14

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.