xsolve-pl-code-review.md

#GLOBAL:

• .DS_Store do .gitignore + wyszyszczenie repo z tych plików

• theme powinien się nazywać inaczej niż 'starter'

• brak 404

• Wyczyście folder z theme z pozostałości po roots.io. Jest tam kilkanaście plików które nie są używane.

• Pozostał także readme.md z roots.io z nieaktualnymi instrukcjami. Myślę że można go usunąć i dodatkowo rozszerzyć readme file w projekcie o instrukcje w jaki sposób postawić projekt.

• Formatowanie. Ogólnie nie ma żadnej konsekwencji. Przyjmijcie jedną konwencję i się jej trzymajcie. Teraz jest mały bajzel w plikach. Np: https://raw.githubusercontent.com/kkoscielniak/xsolve-pl/home-page/wp-content/themes/starter/assets/sass/_home.scss

• folder sass zawiera tylko pliki .scss. Mylące nazewnictwo

#HTML: *

<div role="document">
    <main class="main" role="main"> 
        ... 

to nieostylowane kontenery które są pozostałościami po roots.io. Można spokojnie zrezygnować z tych elementów i w ten sposób zmnieszy się liczba zagnieżdżeń.

• jQuery powinno być ładowane na samym końcu strony. (true w jednym z parametrów w wp_enqueue_script w scripts.php)

• W tagu html język jest ustawiony na: lang="pl-PL", a strona jest po angielsku. Do zmiany.

<meta name="viewport" content="width=device-width, maximum-scale=1">

wywalić maximum-scale. Nie używajcie tego parametru.

• Fallback do jQuery jest ustawiony na nieistniejący plik wp-content/themes/starter/assets/vendor/jquery/dist/jquery.min.js

• API do google.maps lepiej zamieścić na końcu strony, obecnie jest w HEAD. Funkcja inicjalizująca mapę powinna być przeniesiona do scripts.js, obecnie jest w HEAD.

<!--[if lt IE 8]><div class="alert alert-warning">...

Nie jest ostylowane i jest zupełnie niewidoczne na starych przeglądarkach. Jest także trochę za nisko w kodzie. Przenieście to na samą górę i ostylujcie.

• Istnieje plik .editorconfig, ale nie wszyscy macie poinstalowane pluginy które z niego korzystają i stąd np. wcięcia w kodzie nie zawsze są takie same.

• Nawigacja powinna znajdować się w tagu <nav> ... </nav>.

• Prawa część strony to idealny kandydat do <aside></aside>. Zamiast tego w kodzie jest pusty node

<aside class="sidebar" role="complementary"></aside>

• Brak użycia semantycznych tagów (section, article, aside).

• Maile i numery telefonów nie są podlinkowane.

• <a href="#" class="footer__social-icon"><img src="/wp-content/themes/starter/assets/img/svg/facebook.svg"></a> brak fallbacku dla przeglądarek bez svg.

• Brak alt-ów w <img>

• Na stronie jest jeden <h1> i bardzo dużo <h2>.

#JS:

• Brak konwencji w nazewnictwie klas.

    NAV: $('.js-mobile-navigation'),
    CLIENTS_CONTENT: $('.jsClientsContent'),

Albo camelCase, albo hyphen.

• Większość zmiennych w XSolve.VARS odnoszą się do CLIENTS. Przemyślcie czy nie lepiej je przenieść do funkcji która ich używa, a zostawić tylko jeden nadrzędny element (np. CLIENTS_CONTENT). Wtedy wywołanie funkcji też będzie krótsze.

• Grunt jest po części skonfigurowany, ale są tam posostałości po roots.io. Wyczyście go i zostawcie tylko te taski które faktycznie są używane.

• konfiguracja mapy: scrollwheel: false to ustawienie powinno być tylko dla urządzeń mobilnych. Desktopy powinny mieć możliwość zoomowania mapy.

$(window).resize(changeBackgroundSize); Nie używajcie takiego bindowania. Zamiast tego użyjcie jquery.debounce. W tym momencie funkcja jest wywoływana mnóstwo razy na evencie resize.

• backgroundSize = 1400/3 + (windowWidth - 1400)/2 - 30; Dziwne przeliczenia. Czym jest 3, a czym jest 30? Albo zapisujcie takie wartości do zmiennych, albo komentujcie kod aby było wiadomo do czego odnoszą się te wartości.

logo.on('click', function(e) {
    var container = $(this).data('container'),
        author = $(this).data('author'),
        quote = $(this).data('quote'),
        note = $(this).data('note');
        ...
        $(this).addClass('logo--active');

pisałem już o tego typu zapisach. Lepiej jest zapisać $this i do niego się odwoływać. W przypadku który jest powyżej objekt jQuery jest niepotrzebnie tworzony 4 razy.

• XSolve.dynamicBtn block try {} catch {} w tej funkcji niespecjalnie ma sens. Nie ma obsługi błędów.

• W projekcie są dwa pliki o tej samej zawartości. _main.js i scripts.js.

• Formatowanie w pliku mogłoby być lepsze.

• Nie komitujcie zakomentowaneg kodu

#CSS:

• wszystkie wystąpienia &:hover { powinny mieć przedrostek .no-touch aby nie był on widoczny na urządzeniach dotykowych.

• Bardzo dużo breakpointów w _vars.scss Nie sądzę że wszystkie są potrzebne.

• 5 różnych fontów w projekcie?

• Mieszacie jednostki px i em.

• Jest kilka wystąpień tego typu: color: #000;. Powinny one być zastąpione zmiennymi.

• Plik global.scss nie powinien zawierać styli odnoszących pisię do nieglobalnych elementów.

• .project__box { a później .no-touch .project__box { - można to zrobić efektywniej, tzn:

.project__box {
	... 
	.no-touch & {
	...
	}
}

• .page-container jest niepotrzebnie zdefiniowany w kilku miejscach. ( _wrappers.scss, _globals.scss )