Hello, friends.
As promised, here is a code review of the Nation Pizza site. Overall, very nice job! When reading my comments below, please bear in mind that I don’t know the entire context of this project, so I may well be unaware of certain constraints that render my suggestions moot. Still, hopefully there’s some items of use in this-here unordered list!
Cheers,
Dan.
- The
meta charsettag should be directly after the openingheadtag; most browsers will re-start parsing the page after encountering this tag. - The “title” attribute on
atags should give more information on what the link is for, not what the link contains (e.g. a title of “Nation Pizza Logo” on the header link is probably incorrect, vs. the more useful “Click to go home” or even “Home”). - I would recommend moving the About page’s timeline into a separate HTML file to include, so that the context of
internal.htmlcan be grokked more easily. This is probably also true for items withininternal.htmlas well. - Most
scripttags should be placed at the bottom of the page, right before the closingbodytag. See: http://developer.yahoo.com/performance/rules.html#js_bottom
- CSS should be minified. If you are using compass, this is possible by using a config file, see: http://compass-style.org/help/tutorials/configuration-reference/ There are other ways to achieve this, too, but…
- …using compass (or another library) will allow for the use of mixins. These can shrink your code and make sure you don’t forgot any vendor prefixes, e.g. using
@include background-size('contain');will expand for all relevant browsers. - Selectors that include IDs do not also need an elementr name, i.e. use
#mastheadinstead ofheader#masthead - Consider using classes instead of CSS selectors with a large number of IDs, e.g.
.content-container { width: 1030px; margin: 0 auto; clear: both; }instead of those same rules onheader#masthead, section#content, section#capabilities, section#home-slider, footer. In this way, you start to define your rules more on how a thing acts vs. what a thing is. This makes for more modular and reusable CSS. - Consider splitting apart your SCSS files into a greater number of SCSS partials, using logical groupings, e.g. jScrollPane, timeline, home-slider, etc.
- If you’re going to add some sort of CSS hack, consider adding a URL to an explanation of said hack.
- Be careful not to nest your CSS too deeply, and be careful about rules that start from a top-level class/ID on
htmlorbodytags; you likely have increased your depth of applicability and made the CSS less flexible, see: http://smacss.com/book/applicability
- Javascript files should be concatenated together and minified. There are build scripts and libraries that will do this for you in most every language or platform in which we work. See: http://developer.yahoo.com/performance/rules.html#minify and http://developer.yahoo.com/performance/rules.html#num_http
- If you are going to write your own JS slideshow instead of the many existing ones that exist and seem to work exceedingly well, please document the design decisions as to why you did so. If you have taken slideshow code from elsewhere, consider putting it in its own file with proper attribution/license.
- Complex conditionals in the view should be moved into other code, e.g.
if ( ( $page->section !== 'Capabilities' ) || ( ( $page->section == 'Capabilities' ) && ( $page->depth > 1 ) ) )would be better asif ( $page->should_show_quick_links() ) - Consider using some simple PHP code to make markup more DRY. e.g. lists, repetitive markup, etc.
- Add documentation to all PHP classes stating their purpose, and to all PHP functions starting their function, their parameters, and their return values.
- Remove files from the project that are not used, e.g. extraneous JS files like swipe-1.0.min.js
- Optimize images (I did not check to see if this was already done or not) by using libraries like pngquant, imageoptim, etc.