Forums

JoshWhiteDesign

Hi, I'm pretty new to jQuery as a whole and really have appreciated the help and feedback of these forums. I just was hoping someone could tell me if this is the best way to do this. I finally got this thing working (haven't added all the sweet stuff to it).

http://www.citrusbunny.com/
*It's the "Contact Us" on the right side - supposed to slide out*

through my research it seems there are a ton of different ways to get jQuery to do what you want. Was this the best way to execute this? Should I have done it differently?

I just want to make sure I'm doing it right rather than "just making it work".

This is the code for the slide:

stylesheet.css


li.action {
	font-family: georgia;
        font-size: 20px;
        color: #777;
        margin-left:15px;
}
li.action:hover {
        color: #000;
}
        #contactslide {
		height: 200px;
                width: 250px;
		background-color:#ffffff;
                margin-left: 40px;
                padding:0px;
                margin-top:0px;
                margin-bottom:0px;
}

indexcustom.php

<ul>
<li class=\"action\">Contact Us</li>
</ul>
        <div id=\"contactslide\">
              <p>520-555-5555</p>
        </div>

Header.php

 
<?php wp_enqueue_script(\"jquery\"); ?>

		
		<?php wp_head(); ?>


<script type=\"text/javascript\"> 
jQuery(document).ready(function(){
        jQuery(\"#contactslide\").hide();  
	jQuery(\"li.action\").click(function(){                               
                jQuery(\"#contactslide\").slideToggle(\"slow\");
		jQuery(this).toggleClass(\"active\"); return false;
	});	 
});
</script>

tomwalters5678

Nope - that looks just fine! The only important thing is that it makes sense to you and anyone else that will be coding on the site.

Mr KiTT3N

Few things to note, avoid using inline styling


<p style=\"text-align: center;\">For questions of any kind, please call us Monday - Friday 8am to 5pm. Otherwise email us and we will contact you in 2 shakes of a lambs tail.</p>
<p style=\"font-family: georgia; color: rgb(119, 119, 119); font-weight: bold; font-size: 30px; text-align: center;\">520-205-2306</p>


<p>For questions of any kind, please call us Monday - Friday 8am to 5pm. Otherwise email us and we will contact you in 2 shakes of a lambs tail.</p>
<p id=\"contactNumber\">520-205-2306</p>




// css file
div#contactslide p                                 { text-align: center; }
    div#contactslide p.contactNumber     { font-family: georgia; color: rgb(119, 119, 119); font-weight: bold; font-size: 30px;}

Its bad practice and clean code is just better for the eyes ;)

2nd thing

In css its encouraged to use as many elements to select one (I.E. == html body div.content div#nav li.action {....)
When using jquery, its just the opposite

 jQuery(\"li.action\").click(function(){....

What this is doing is selecting every single <li> on the page.... stores them and then filters all those <li> looking for the class name "action"

note: Selecting by a class is slower than id

3rd thing

<li class=\"action\">Contact Us</li>

This is the only element with the class of "action", Classes generally apply to multiple elements and this is the clearly the only li that is going to have the action therefor should be an id

The Element is also supposed to be click able and you dont get the pointer when hovering fixable by css (cursor: pointer;) or better yet

<a href=\"#\" id=\"action\">Contact Us</a>

It is supposed to be a clickable element its always good to not reinvent the browser recreating the way it defaultly acts

4th Thing

Notice the little jump at the end of the slide down and beginning of the slide up?


#contactslide {
background-color:#FFFFFF;
height:200px;
margin-bottom:0;
margin-left:40px;
margin-top:0;
padding:0;
width:250px;
}

remove the height, it will fix that


#contactslide {
background-color:#FFFFFF;

margin-bottom:0;
margin-left:40px;
margin-top:0;
padding:0;
width:250px;
}

5th thing....

It seems like you could remove a handful of elements that dont need to be there (ie div id='lower') that just has the height of 5px... you can just apply top margin to the header element

And im tired....

treehouse : what would you like to learn today?

Forums

Please critique my technique