Skupię się na defektach, praktycznie pomijając to, co jest dobrze (dobrze, że HTML5, że nie ma błędów innych niż alty, brak tabel, że kod prosty póki co etc. etc.).
[edit]
Disclaimer: może się wydawać, że pojadę trochę ostro i może faktycznie tak będzie. W sensie: będę się czepiał również drobniejszych rzeczy. Uznałem jednak, że dokładniejsza recenzja kodu frontendowego może Ci się tylko przydać, bo Cię przecież nie zabije ;). Ja na co dzień pracuję jako frontendowiec. Recenzje kodu robię naprawdę często. Pracuję jednak w bardzo mocnym zespole samych frontendowców -- kolesi, którzy SKUPIAJĄ SIĘ na HTML-u, CSS i JavaScripcie. Staramy się uzyskać zawsze najwyższą jakość i przyczepiamy się do rzeczy, które inni by olali.
Możesz wziąć to pod uwagę. To tak żebyś nie myślał, że kod jest gorszy niż jest. Szczególnie, że wiem @somekind, że piszesz też (głównie) kod backendowy. Na pewno widywałem dużo gorsze kody HTML/CSS pisane przez backendowców ;) (to tez jest naturalne: wyobraź sobie, że koder CSS zabiera się nagle za C#...).
[/edit]
- Nie waliduje się ( http://validator.w3.org/ ). Myślisz, że walidacja to bzdura? Te 4 błędy, która wywala walidator, związane są z kolejnym punktem, bardzo poważnym i realnym błędem funkcjonalnym dla części użytkowników.
- Brak odpowiedników tekstowych dla głównej nawigacji, logo i motto. Potężny problem dostępnościowy. Google tego nie zindeksuje.
- Na stronach innych niż główna, logo powinno być linkiem do strony głównej (znana, wygodna konwencja).
- Brak
- Prezentacyjna nazwa id:
- Elementy czysto prezentacyjne, co najmniej wielu z nich dałoby się uniknąć dzięki lepszym stylom:
<div id="righttop"></div>
<div id="top"></div>
<div id="bottom"></div>
<div id="footer">
<div id="leftfoot"></div>
<div id="rightfoot"></div>
</div>
div#lefttop
div#righttop
Samo ID by stykło, po co ten div?
- Duża specyficzność w regułach z dwoma ID -- a to niepotrzebne, chyba że kontekstowo:
div#main div#top
div#main div#center
- W ogóle w większych projektach spamowanie ID-kami się nie opyla ze względu na wysoką specyficzność selektora i niską reużywalność elementów z ID. To jest malutki projekt, więc to jeszcze nie boli.
- A ten selektor znowu w większym projekcie (a i ten się rozrośnie!) byłby zbyt ogólny:
nav ul li
Witryna może mieć wiele elementów <nav>. Dałbym tu jakąś klasę lub ID, np. main-nav.
- Czemu nie użyto własności zbiórczej background?
background-image: url(./img/clouds.png);
background-position-y: top;
background-repeat: repeat-x;
- Elementy #top, #bottom i znajdujące się na nich obrazki są zbędne. Zamiast tego, wystarczyło użyć border-radius z CSS3. Stare IE można olać albo można skorzystać z CSS3 PIE.
- (+) Dobrze, że skorzystano z <body> żeby zakotwiczyć jedno tło...
- ...ale nie skorzystano już z <html>.
- Stopkę i/lub nagłówek trzeba było oprzeć na elementach otaczających (wrapperach), w tym na <body>/<html>, a nie pozycjonowanych bezwzględnie elementach prezentacyjnych. Dół strony potrzebuje tylko 2 elementów, bo są 2 tła. Tutaj są 3 elementy.
- Strona się nie mieści w poziomie w mniejszych rozdzielczościach. Niepotrzebnie. To przez te elementy prezentacyjne. Gdyby zamiast nich użyć luźnych wrapperów, bez zdefiniowanej szerokości, nie byłoby takich problemów.
- Czyszczenie (clear) nie działa gdy używamy pozycjonowania absolutnego, jest tu więc zbędne:
clear: both;
position: absolute;
@MVC:
U niego ta "stopka" jest pusta. Semantycznie, nie istnieje. Znacznik <footer> nic tu nie da, a nawet będzie sugerował, że coś tam jednak powinno być. To nie jest tak naprawdę stopka, tylko obrazek na dole, bez żadnej treści.
edit:
Ogólne porady, frontendowa droga rozwoju:
-Semantyka. Polecam zapoznać się z tym terminem i zacząć go "czuć". HTML to semantyka. Podkreśla strukturę dokumentu. Nie jego prezentację, czyli wygląd.
-Pozycjonowanie w CSS. Tak jak napisał @dzek69, standardowym problemem ludzi zaczynających przygodę z CSS jest nadużywanie pozycjonowania bezwzględnego. Ono nie jest tak dobre jak się z początku wydaje. Zachęca do spamowania pustymi elementami prezentacyjnymi (czasem trzeba je dodać, ale coraz rzadziej), no i przede wszystkim jest bardzo nieelastyczne. Polecam częściej korzystać z elementów otaczających, tj. otoczyć jakiś element kolejnym, tzw. wrapperem, i w nim dać dodatkowe tło. To jest zwykle lepsze niż pusty element. Ale tak naprawdę, dodatkowe wrappery i puste elementy to już prezentacja w HTML-u, więc starajmy się to minimalizować.
