Ocena projektu strony - poprawki.

Question

Cześć,

mam pytanie, ponieważ zrobiłem projekt strony na podobieństwo innej strony i prosiłbym bardzo aby ktoś ocenił kod. Wiem, że posiada dużo błędów i gdyby ktoś mógł wypunktować rzeczy do poprawienia to byłbym wdzięczny. Staram się o stanowisko juniora, ale bez mentora jest strasznie ciężko.

https://github.com/ItsShock/page-aa

Pozdrawiam, z góry dziękuję za pomoc.

Comments

Daj link do wersji live...

Answer 1

Sugestie do rzeczy, które rzuciły mi się w oczy.

HTML:

• https://github.com/ItsShock/page-aa/blob/main/index.html#L10 czemu nie podepniesz tego jako po prostu jako CSS, tylko używasz import?
• https://github.com/ItsShock/page-aa/blob/main/index.html#L14 skoro to jest główny skrypt na stronie, to wrzuć go na koniec </body> albo ustaw atrybut defer
• dlaczego <img> mają pusty atrybut alt? Powinien być tam krótki opis tego, co przedstawia obrazek. Jeśli obrazek jest ozdobnikiem, który nie ma znaczenia dla treści, to wstaw ten obrazek poprzez CSS jako background
• https://github.com/ItsShock/page-aa/blob/main/index.html#L42 jeśli ten element pełni rolę jakiegoś separatora contentu, to użyj CSS-a (margin, border), ewentualnie znacznika bardziej generycznego, jak <div>, zamiast używać pustego <p>
• https://github.com/ItsShock/page-aa/blob/main/index.html#L48 gdzie na tej stronie są nagłówki h1 i h2?
• https://github.com/ItsShock/page-aa/blob/main/index.html#L414 ta sekcja wygląda jak jakaś nawigacja, więc  czemu nie ma tutaj <nav>? Znaczniki wewnątrz wyglądają na listę, więc czemu nie ma <ul> lub <ol>? Na oko nadużywasz <div> i <p> - poćwicz stosowanie odpowiednich znaczników semantycznych (klik, klik)

JavaScript:

• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L1 ten even listener będzie zbędny, gdy umieścisz skrypt na przed </body> lub ustawisz mu atrybut defer
• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L3 używasz let tam, gdzie z powodzeniem możesz użyć const. Reguła kciuka: jeśli do danej zmiennej przypisujesz wartość tylko raz (w momencie deklaracji), to używaj const
• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L27 jeśli taki element jest tylko jeden, to pobierz go poprzez querySelector i nie będziesz musiał odnosić się do niego indeksem
• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L23 czemu tutaj nadpisujesz zmienną z linii 9? To powinno rzucać błędem, bo nie można redeklarować zmiennych let w obrębie jednego scope
• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L32 zakładam, że jednak liczba jest docelowym typem dla zmiennej newPrice (a nie kolekcja DOM z linii 9). Więc uspójnij kod, bo w tym miejscu nieoczekiwanie przypisujesz do tej zmiennej całkiem inną wartość (kolekcja DOM vs liczba)
• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L64 powtarzający się kod (przykładowo, lnie 39-40 oraz 64-65) dobrze jest wydzielić do osobnych funkcji
• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L73 ryzykownie jest używać var w pętli for, gdy wewnątrz jest kod asynchroniczny, ponieważ wartość indeksu w kodzie asynchronicznym będzie wskazywać na tą z ostatniej iteracji. Lepiej var w nagłówku pętli zmienić na let lub przynajmniej owinąć kod w IIFE
• https://github.com/ItsShock/page-aa/blob/main/js/main.js#L92 (taka grupa kodu się powtarza) jeśli nie potrzebujesz dynamicznie określać styli z JS-a, to ostyluj element w CSS, a w JS tylko manipuluj klasami

CSS:

• powtarzalne style możesz zgrupować selektorem listy
• powtarzalne wartości dla styli możesz przypisać do zmiennych CSS
• https://github.com/ItsShock/page-aa/blob/main/css/style.css#L145 lepiej unikać stylowania po ID (klik)