Jump to content

Finished A Site And Need Some Constructive Criticism


Recommended Posts

I'm practising my html and css. So I searched around for a cool looking PSD to slice up and put together. Here's what I came out with.

programs I used were:

Ubuntu 12.04



and some button generator


Any tips or any constructive criticism is greatly appreciated


in case you didn't want to download my .zip file here are links to my code and the original



My CSS code


what the original looks like


What do you think?


post-69424-077912200 1365667792_thumb.jpg

Link to comment
Share on other sites

I uploaded your files as they are (minus path adjustments that have nothing to do with your code, only how/where I save these forum pages) and I get: http://aandbwebdesign.com/KSforum/polo.html


Overall, it looks very good for a pretty complex page. Maybe some adjustments around the top navigation.


Next, I ran the page through the validator - also good, only 9 errors.


I honestly have no idea what the problem with the closing head tag is supposed to be, it looks good to me (but then, I don't have all the answers).


The next error about the duplicate id tag is simply because IDs are meant to be unique per page, meaning you can use each only once. Use a class if you need something more than once.


The line 46 error, you have a quote misplaced. alt=css button generator"lady jumping" should be alt="css button generator lady jumping" -- or probably better: alt="lady jumping"


The alt tag is so you can describe the image for those who cannot see it.


The rest looks pretty good. Some minor things are for example your footer - you should not need the footer div and the footer_container div inside that. You can apply any styling directly to the footer div itself.


In your CSS you have some redundancies - example, the reset css applies the zero padding, margin, and border to a whole list of elements. There is no need to repeat this zero setting on the other sheet (or ever). Also, there really is no need for a separate reset CSS, you can paste all that stuff to the top of your style css. Nothing wrong with multiple css documents, but it might make more sense if you have a very complex navigation to keep it separate - for example.


Anyway, just a couple pointers, hope it helps - let me know if you have an questions.

Link to comment
Share on other sites

Just a little more for your css zeroing out you can simply put * {margin:0; padding:0;} The * is what is called a wild card and effects all of the html Dom elements. If you not understand what the Dom is that is ok you will when you start writing jQuery. For now just know that it effects everything on the page. Also html5 elements are block by default, not that it hurts to declare then anyway. But less code is always better.


You should add this to the head just before the closing head tag, anyway that is where I put it but as long as it is in the head. The code is for IE as you can tell because older IE does not like html5.


<!--[if lt IE 9]>
<script src="http://html5shiv.googlecode.com/svn/trunk/html5.js"></script>



Also good job on the id's and classes, exempt for the one as Andrea said. So many people want to use only classes all of the time where id's are better.


I would give you 93% an A clap.gif

Edited by grabenair
Link to comment
Share on other sites

I was in such a hurry to post this I completely forgot to validate :bash: I would have cleared most of that up. Sorry about that. On line 46, you are correct, it was just supposed to be "lady jumping". that button generator must have been some kind of copy and past accident.



The rest looks pretty good. Some minor things are for example your footer - you should not need the footer div and the footer_container div inside that. You can apply any styling directly to the footer div itself.


About the footer. I put the footer div for the background image to go side to side 100% of the way, but used the inside container to keep everything inside the 940 width. How would I go about cutting this down to just one div? I could have the id=footer and class=container on one div, but how would I make that image go from side to side?

Now I look at it again I really didnt need to put id=footer_container.


Thank you all for being so helpful!

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

  • Create New...