Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am very new to PHP and I kind of sort of want to create the pages like I would in ASP.NET; it's just the way I am wired. I have been inserting the header/menu using a PHP Include and here is what the code looks like for just the first page.

Am I doing this properly, or is there a better way?

index.php

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <title>New Jerusalem</title>
    <link rel="stylesheet" type="text/css" href="Main.css"/>
  </head>
  <body>
    <div id="wrapper">
      <div id="header">
        <?php include ('Menu.php') ?>
      </div>
    <div id="content">
      <h1 class="PageTitle" >Sturgis Hellfighters</h1>
        <div class="indexright" width="50%">
          <h3 class="smallheader">Scripture Corner</h3>
          <p><img src="images/Romans5-8.jpg" width="475" alt="Romans 5:8" /></p>
          <p></p>

        </div>
        <div class="indexleft" width="50%">
          <p><img src="images/BridalFallsSpearfishCanyon.jpg" width="475" height="720" alt="Bridal Falls Spearfish Canyon" />
          </p>
        </div>
      </div>
    </div>
  </body>
</html>

Menu.php

<img src="images/SiteHeader.jpg" width="1000" height="150" alt="HellFighters New Jerusalem" id="siteheader"/>
<ul id="HomeMenu">
    <li class="TopLevel"><a href="index.php">Home</a></li>
    <li class="TopLevel"><a href="#">News</a>
        <ul>
            <li class="SecondLevel"><a href="#">Sturgis Current Events</a></li>
            <li class="SecondLevel"><a href="#">Rapid City Events</a></li>
            <li class="SecondLevel"><a href="#">Pierre Events</a></li>
            <li class="SecondLevel"><a href="#">Other Events</a></li>
        </ul>
    </li>
    <li class="TopLevel"><a href="#">Photos</a>
        <ul>
            <li class="SecondLevel"><a href="christmasatpineridge.php">Christmas On Pine Ridge</a></li>
            <li class="SecondLevel"><a href="XmasMission.php">Christmas At the Mission</a></li>
            <li class="SecondLevel"><a href="OpenHouse.php">Open House</a></li>
            <li class="SecondLevel"><a href="Nationals.php">Nationals</a>
        </ul>

    </li>
    <li class="TopLevel"><a href="#">Events</a>
        <ul> <!--  -->
            <li class="SecondLevel"><a href="#">A Pine Ridge Christmas</a>
            <li class="SecondLevel"><a href="#">A Sturgis HellFighter Christmas</a>
            <li class="SecondLevel"><a href="#">Sturgis Motorcycle Rally</a></li>
            <li class="SecondLevel"><a href="#">Random City Swap Meet</a></li>
            <li class="SecondLevel"><a href="#">Cool Event</a></li>
        </ul>   
    </li>
    <li class="TopLevel"><a href="#">Contact Info</a>
        <ul>
            <li class="SecondLevel"><a href="#">President</a></li>
            <li class="SecondLevel"><a href="#">Vice-President</a></li>
            <li class="SecondLevel"><a href="#">Preacher Man</a></li>
        </ul>
    </li>    
</ul>

I would like to use HTML5 if possible, so if your review brings up HTML5 that is cool with me too.


If you would like to see the front page, click here.

This page will be changing as I am currently working on it.

share|improve this question
    
I was surprised to see that you had a working example, <?php include ('Menu.php') ?> I would think would generate a fatal error with the semicolon. –  Pickett Feb 2 at 20:51
    
@Pickett what do you mean? because I didn't put a semicolon at the end? I was following some examples. they could have been old examples... –  Malachi Feb 2 at 20:54
2  
Sorry, my bad. Your syntax is OK, however I would still end each statement with a semicolon. –  Pickett Feb 2 at 20:57
    
@Pickett I agree, I like Semicolons. –  Malachi Feb 2 at 20:57
1  
Pedantic note/tip: if menu.php doesn't contain any PHP, then change its extension to what it really is: html. The php extension means your sever is passing the file to the PHP parser first, which doesn't do anything but take up more resources & slowing you down. the PHP extension is only required for files that contain code –  Elias Van Ootegem Feb 3 at 13:10
show 1 more comment

2 Answers 2

up vote 11 down vote accepted

Going to some HTML5 stuff straight away.

  1. Replace your doctype with the HTML5 doctype. This is necessary for the next few steps

    <!DOCTYPE html>
    
  2. Use the shorter charset meta tag and the meta viewport tag inside your head area. The viewport meta tag is necessary for mobile devices and ensures a good default viewport.

    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    

    Side note: You may omit the type-attribute for style, script and link tag (not for RSS) and the closing / for self-closing tags. This is not a recommendation, but it's possible.

  3. Use ID's only when you definitely can say "There will be only one element of this per page, ever". A wrapper is not unique to a page. It should be a class.

    Generally speaking, ID's don't provide you with a true benefit and kinda makes your life harder. I don't use ID's as styling hooks anymore and I'm really happy with it.

  4. You're navigation is included with PHP and is named menu.php, but it actually contains a header image as well. A better name would be header.php, wouldn't it?

    That being said, you may consider moving everything above your header and the header itself to a header.php. Even your head area and the html tag. This is only usable, if you your header is the same on all of your pages.

  5. I'm using dash-delmited class names in my HTML. No CamelCase names. This is my preference and if you want to use CamelCase, this is fine. However I would avoid using no delimiting and lowercase names like indexright or smallheader.

That's it for now. I'll going to edit some more stuff later. Stay tuned.

share|improve this answer
add comment

In addition to kleinfreund suggestions :

  1. For a link to the homepage, i'd rather do

    <a href="/">Home</a>
    

    instead of

    <a href="index.php">Home</a>
    

    as http://sturgishellfighters.hartwebpages.com/ is a cleaner URL than http://sturgishellfighters.hartwebpages.com/index.php

  2. If you care about W3C validation, you should remove the width attribute of the divs and use CSS achieve the same effect.

  3. For stronger semantic, you can use HTML5 sectioning elements instead of divs.

    Like <header> instead of <div id="header">,

    <main id="content"> instead of <div id="content">,

    and wrap your main navigation <ul id="HomeMenu"> with <nav>

  4. For more accessibility, you can add ARIA landmark roles to the tags mentioned above :

    <header role="banner">
    <nav role="navigation">
    <main id="content" role="main">
    
share|improve this answer
    
the width in the tags is a good one, I normally don't do that, I don't know why I did that this time. as for the URL I am not too familiar with redirects on the server, but that is something that I would like to set up too, I think that is what makes this happen where the URL doesn't change when you change the pages. –  Malachi Feb 9 at 1:34
    
of course I really don't like creating a class for one specific item though either, it makes more work for myself, so it it is one thing on one page then I would probably set the width in the HTML, because it isn't really styling it's structure. –  Malachi Feb 9 at 21:20
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.