Feedback & Code Review od Mentora Recenzenta (Hubert Legęć) #100

MateuszNaKodach · 2021-01-19T20:56:32Z

No description provided.

Bumps [node-notifier](https://github.com/mikaelbr/node-notifier) from 8.0.0 to 8.0.1. - [Release notes](https://github.com/mikaelbr/node-notifier/releases) - [Changelog](https://github.com/mikaelbr/node-notifier/blob/v8.0.1/CHANGELOG.md) - [Commits](mikaelbr/node-notifier@v8.0.0...v8.0.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.8. - [Release notes](https://github.com/isaacs/ini/releases) - [Commits](npm/ini@v1.3.5...v1.3.8) Signed-off-by: dependabot[bot] <support@github.com>

…rmatting #4 Automatic precommit code formatting

* #28 Logika | Zapisywanie rankingu w Local Storage * #28 Changes after review * #28 Changes after review Co-authored-by: Anna Lamperska <anna.lamperska@nokia.com>

#9 UI Komponent | Logo Star Wars

Zmiana wprowadzona, gdyż zamiast korzystania z szablonu html użyjemy renderowania komponentów w JavaScript.

* #27 method and tests checking the player answers * #27 correct the name of the tests * #27 update file names * #27 shorten the return function Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl> Co-authored-by: Mateusz Nowak <kontakt.mateusznowak@gmail.com>

…ples

…g principles" This reverts commit fcdd2fa

* Add favicon * Fix favicon links in index.html * Add favicon 192x192 and 512x512 in html

…ples

* Create render function Create renderTemplate.js file with button rendering function * Add div button in DOM Create new div for button rendering, add local history folder to gitignore * Add styles to rendered button Add style rules for button and font * Refactor render function * #13 Button render function Button render function can render all needed buttons * #13 Add second icon to static/ui Create second icon and add to static/ui, change icons styles * #13 Tests and code refactor Add tests for Btn function and refactor code * #13 Quick fix - file name * #13 Rename btn.js to Btn.js * #13 Button click test Add test that checks if button was clicked * #13 CR fixes * #13 CR fix 2 * Fix class names * Update text size * Add hover * Render Btn in App.js Co-authored-by: PiotrWR <pwrynio@gmail.com>

#14 Add GameMode component

…nent inside another It's also possible to assign additional class by this function.

* Finish common Btn * Fix - delete 'RedBtn" @import from style.scss * Add isSpecial proprety in Button.js * Fix tests * Update "isSpecial" value from "" for "false" * Add $color-bg--special var in _vars.scss * Add 2 tests * Add Correct & InCorrect Answer

* Part of work * Finished Computer Mind * Create computerMind.js * Fix computerMind * ComputerMind answer on asked question to computer player Co-authored-by: Mateusz Nowak <mateusz.nowak@vonage.com>

* Quick fix Add if statement in render function to prevent adding undefined class to elements classList * Render function refactor + tests * fix test refactor * Delete unused import

* #18 create modal window * #18 create scss variables for components * #18 solve an error with variable * #18 center content of the modal window * #18 problem with calling the close function * #18 add show and close functions * #18 review fixes-change MW from class to function * #18 remove comments, add tests * #18 remove button from index.html * #18 update with var from develop * #18 change modal window tests * #18 update app.js file Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

Co-authored-by: lamparina <anna.lamperska@gmail.com>

* pull develop * #19 create modal window content component * #19 create modal window content * #19 require the input + rewite all using render * #19 problem with functions * #19 fix for functions * #19 fix errors due to window close * #19 use filter instead of for loop * #19 tests - mission impossible * #19 tests update * Add Modal Window tests * Add Modal Window tests * Modal Window - listener on form submit * #19 fix for image * #19 comment unnecessary button * #19 test fix * #19 shorten thecorrectAnswersCounter function Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl> Co-authored-by: Mateusz Nowak <mateusz.nowak@vonage.com>

* #10 UI Component - players ranking Add component witch takes an top scores array and renders its content * #10 Test render of ranking component * #10 Rename component classes * #10 Refactor * #10 Refactor code and styles * #10 Tests * #10 Tests Add component tests * #10 After CR tests fix * #10 code refactor

@PiotrWr

… ) (#80) Witamy, aktualnie istnieje pełna wygląd oraz obsługa mainContainera w obejmującym "Rules" oraz "Raning". Kod został poprawiony, jednakże nie jest on najpiękniejszy. Występują uzasadnione obawy, że nie zdążymy upiększyć kodu przed deadline. Dlatego też udostępniamy w pełni działający kod. Aktualnie pracujemy też nad dodaniem dodatkowej funkcjonalności (renderowanie i obsługa widoku Quizu), która okazała się NIEZBEDNA, a nie jest ujęta w task'u, który mieliśmy zrobić (ani w żadnym innym task'u z projektu). Ta funkcjonalność zostanie napisana już elegancko ;) Pozdrawiamy @PiotrWr & @tomdworniczak ------------------------------------------ * #23 Add test of StarWars Triller (from PiotrR, TomaszD, PawełS) * Finish Start Window Component (Piotr Rynio i T Tomasz Dworniczak) * Add more time for StartWindow animation (Piotr Rynio & Tomasz Dworniczak) * Add TODO in Wrapper ( @PiotrWr & @tomdworniczak ) * Not Full Commit * Add wrappet tests TDD ( @PiotrWr & @tomdworniczak ) * Fix wrapper tests ( @PiotrWr & @tomdworniczak ) * Add setStartView function ( @PiotrWr & @tomdworniczak ) * Part ( @PiotrWr & @tomdworniczak ) * Next Commit * Next commit ( @PiotrWr & @tomdworniczak ) * Create game mode view switching and buttons service ( @PiotrWr & @tomdworniczak ) * Create ranking retrieving function ( @PiotrWr & @tomdworniczak ) * #23 Create separate view for rendering GameOptionsView ( @PiotrWr & @tomdworniczak ) * Fix 1/2 part (Piotr Rynio i T Tomasz Dworniczak) * Fix 1/2 part (Piotr Rynio i T Tomasz Dworniczak) * Finish Game Options View (Piotr Rynio i T Tomasz Dworniczak) * Uncomented Animation * #23 next part ( @PiotrWr & @tomdworniczak ) * Fix after Code Review

@PiotrWr

Have Fun! ( @PiotrWr & @tomdworniczak )

* Unblock Animation * Added Template Wirtual GameManager Machine! * #22 Add mode titlie generation * Working UI without IMG * Add Change Img Function * #22 Cleaning files * Update GameQuizView * Update names * Add small tests * After CR

* Unblock Animation * Added Template Wirtual GameManager Machine! * #22 Add mode titlie generation * Working UI without IMG * Add Change Img Function * #22 Cleaning files * Update GameQuizView * Update names * Add small tests * After CR * Update Constructor * Finish CReview

* Unblock Animation * Added Template Wirtual GameManager Machine! * #22 Add mode titlie generation * Working UI without IMG * Add Change Img Function * #22 Cleaning files * Update GameQuizView * Update names * Add small tests * After CR * Update Constructor * Finish CReview * Fix of GameQuizView

* #21 Create Timer component and styles Create timer function and render function to it, add styles for timer * #21 Timer styles * #21 Tests * #21 Function refactor * #21 Timer function separation of concerns * refactor * #21 tests refactor * fix * #21 refactor * #21 Code refactor * #21 timer refactor * #21 Refactor * #21 Add another callbackfuntion to MainTimer, apply styles to TextTimer * #21 Fix in callback function * # 21 Add test for TextTimer and MainTimer

* pull develop * fix for loading yoda and death star images Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

* pull develop * Fix - add ranking sort before return * #92 review changes Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

* pull develop * new view for modal window nad its content Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

* pull develop * remove border Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

* pull develop * #73 first try with responsivity * #73 grid change * #73 reposnvity for the main page * #73 responsivity for main page * #73 remove comments * #73 responsivity for start window * #73 responsivity for mode ranking view * #73 responsivity for modalWindow * #73 last fixes * #73 responsivity for answer buttons * #73 remove dead code from _main.scss Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

* pull develop * #73 first try with responsivity * #73 grid change * #73 reposnvity for the main page * #73 responsivity for main page * #73 remove comments * #73 responsivity for start window * #73 responsivity for mode ranking view * #73 responsivity for modalWindow * #74 start * #74 responsivity from 601 to 1024 * #74 responsivity for answer buttons Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

* pull develop * mobile responsivity fix Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

* pull develop * time fixes * fix for tests Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

HubertLegec

Ogólnie podejście do organizacji kodu bardzo mi się podoba. Fajnie że jest tak dużo testów, chociaż samo podejście do testowania proponowałbym trochę zmienić. Więcej testowania zachowania i logiki aplikacji, więcej testów integracyjnych a mniej opierania się na bebechach.
Podsumowując, jak na pierwszy projekt grupowy to jest super.

HubertLegec · 2021-01-20T20:31:11Z

package.json

  },
  "homepage": "https://github.com/CodersCamp2020/CodersCamp2020.Project.JavaScript.StarWarsQuiz#readme",
  "dependencies": {
+    "parcel": "^1.12.4",


nie ma potrzeby dodawania parcela do dependencji produkcyjnych, wystarczy jak będzie w devDependencies

HubertLegec · 2021-01-20T20:31:56Z

package.json

  "homepage": "https://github.com/CodersCamp2020/CodersCamp2020.Project.JavaScript.StarWarsQuiz#readme",
  "dependencies": {
+    "parcel": "^1.12.4",
+    "@types/jest": "^26.0.19",


po co typy skoro to projekt z JSa? Poza tym również powinny być w devDependencies jak już.

HubertLegec · 2021-01-20T20:33:31Z

sass/_mixins.scss

@@ -0,0 +1,12 @@
+@mixin brand-main-title {
+  // display: inline-block;


Zakomentowany kod to nie jest dobry pomysł. Mamy kontrolę wersji więc nawet po usunięciu w razie czego będzie można do niego wrócić.

HubertLegec · 2021-01-20T20:33:52Z

sass/_reset.scss

@@ -0,0 +1,56 @@
+// *
+// * nie dotykać - dobrze jest :)


HubertLegec · 2021-01-20T20:36:50Z

sass/_reset.scss

+}
+
+html {
+  font-size: 10px;


Plus za stosowanie jednostek względnych, ale jednocześnie minus za hardcode bazowego rozmiaru. Przy takim podejściu użytkownik nie ma możliwości zmiany rozmiaru czcionek w ustawieniach przeglądarki. Lepszą alternatywą jest 62,5% (16px * 62,5% = 10px) bo wtedy zależy od ustawień użytkownika.

HubertLegec · 2021-01-20T22:06:19Z

test/computerMind.spec.js

+import { ComputerMind } from '../src/app/ComputerMind';
+
+describe('Computer Mind', () => {
+  const computerPlayer = new Player();


Tu jest tylko jeden przypadek testowy więc wszystko działa, ale generalnie nie powinno się robić mocków czy też definiować stanowych klas w tym miejscu, albo na przykład na poziomie pliku poza describe. Trzeba dbać o to, aby każdy przypadek testowy dostawał świeży stan więc najlepiej robić to w beforeEach

HubertLegec · 2021-01-20T22:08:00Z

test/fetchData.spec.js

@@ -0,0 +1,38 @@
+const { fetchData } = require('../src/utils/fetchData');


a dlaczego tutaj require? trzeba być konsekwentnym w importowaniu i już wszędzie używać import

HubertLegec · 2021-01-20T22:09:26Z

test/gameplay.spec.js

+    gameplay.onPlayerAnswered('C', true);
+
+    expect(gameplay.userAnswers).toStrictEqual(userAnwersAfterQuestion);
+    expect(gameplay._askQuestionToUser).toBeCalled;


ta asercja nie działa, brakuje wywołania funkcji boBeCalled, powinno być:

expect(gameplay._askQuestionToUser).toBeCalled();

HubertLegec · 2021-01-20T22:18:02Z

test/getRandomIdFormArray.spec.js

+
+  const result = getRandomIdFromArray(array);
+
+  let hasDuplicate = result.some((val, i) => result.indexOf(val) !== i);


to sprawdzenie jest błędne, jeśli ustawiłbym:

const array = ['a', 'b', 'b', 'b', 'c', 'd', 'e', 'f'];

to asercja też będzie spełniona, some najprawdopodobniej znajdzie na początku ten sam element.

HubertLegec · 2021-01-20T22:19:24Z

test/layouts/wrapper.spec.js

+  it('When use "setStartView()" function then check viseable of "logo", "menu", "photo","mainContainer" ', () => {
+    const testWrapper = Wrapper();
+
+    const testLogo = testWrapper.querySelector('.logo');


opieranie się w testach na klasach to nie najlepsza opcja, podczas refactoringu styli trzeba przepisywać testy. Jeśli nic innego nie działa, to ostatecznością powinno być testid, ale moim zdaniem nigdy klasy.

PiotrRynio and others added 30 commits December 20, 2020 15:47

Add sass structure!

d8cd4b1

Add sass files -> Fixed last commit

c46db18

Init prettierrc

d1e2753

#3 Configure scss support with Parcel

69e5090

#3 Add background image

ee5add9

Bump ini from 1.3.5 to 1.3.8 in /swapi-json-server

0b19a30

Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.8. - [Release notes](https://github.com/isaacs/ini/releases) - [Commits](npm/ini@v1.3.5...v1.3.8) Signed-off-by: dependabot[bot] <support@github.com>

#38 Add HTML elements structure

058795f

#38 Main view structure rendering driven by JavaScript

440bcbd

#24 Add Player logic for asking and answering questions

7419f71

Add 'Husky' & "pretty-quick"

e9e78ab

Test of working

fbaf571

Finish Logo

618c014

Merge pull request #44 from nowakprojects/automatic-precommit-code-fo…

dd26746

…rmatting #4 Automatic precommit code formatting

Merge branch 'develop' into UI-Komponent-Logo-Star-Wars

a8cc3d5

#28 Logika | Zapisywanie rankingu w Local Storage (#50)

4823ecd

* #28 Logika | Zapisywanie rankingu w Local Storage * #28 Changes after review * #28 Changes after review Co-authored-by: Anna Lamperska <anna.lamperska@nokia.com>

Merge branch 'develop' into UI-Komponent-Logo-Star-Wars

b368f41

Merge pull request #46 from nowakprojects/UI-Komponent-Logo-Star-Wars

7eb4af8

#9 UI Komponent | Logo Star Wars

#47 Delete useless HTML elements structure (#52)

77a6e1f

Zmiana wprowadzona, gdyż zamiast korzystania z szablonu html użyjemy renderowania komponentów w JavaScript.

#8 Add question generator

efc6386

#8 Refactor QuestionGenerator following functional programming princi…

fcdd2fa

…ples

Revert "#8 Refactor QuestionGenerator following functional programmin…

fcc9392

…g principles" This reverts commit fcdd2fa

#45 UI | Favicon.ico

21ed9c6

* Add favicon * Fix favicon links in index.html * Add favicon 192x192 and 512x512 in html

#11_UI_Komponent_Przycisk_czerwony (#49)

8507d4a

#8 Refactor QuestionGenerator following functional programming princi…

6cec9c0

…ples

#14 Add GameMode component (#55)

b2474d5

#14 Add GameMode component

#59 Add common render function, which can be used to render any compo…

b71ffcd

…nent inside another It's also possible to assign additional class by this function.

Common btn (#62)

405d888

* Finish common Btn * Fix - delete 'RedBtn" @import from style.scss * Add isSpecial proprety in Button.js * Fix tests * Update "isSpecial" value from "" for "false" * Add $color-bg--special var in _vars.scss * Add 2 tests * Add Correct & InCorrect Answer

PiotrRynio and others added 29 commits January 11, 2021 00:31

#32 Computer Mind

2e451e6

* Part of work * Finished Computer Mind * Create computerMind.js * Fix computerMind * ComputerMind answer on asked question to computer player Co-authored-by: Mateusz Nowak <mateusz.nowak@vonage.com>

Quick fix of render function (#68)

7cd00af

* Quick fix Add if statement in render function to prevent adding undefined class to elements classList * Render function refactor + tests * fix test refactor * Delete unused import

#15 UI Komponent | Zasady gry dla wybranego trybu (#71)

99ee970

Co-authored-by: lamparina <anna.lamperska@gmail.com>

GitHub Actions | Deploy from develop branch

e10f75d

Unblock Animation (#81)

f02a6f5

Have Fun! ( @PiotrWr & @tomdworniczak )

Fix for displaying question image and logo after build

0a26f53

#22 Developed game quiz view (#82)

96d8bdf

* Unblock Animation * Added Template Wirtual GameManager Machine! * #22 Add mode titlie generation * Working UI without IMG * Add Change Img Function * #22 Cleaning files * Update GameQuizView * Update names * Add small tests * After CR

Fix for images (#90) change their src

adc3b5f

* pull develop * fix for loading yoda and death star images Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

Fix/ranking sort added (#92)

dd0e59d

* pull develop * Fix - add ranking sort before return * #92 review changes Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

Update view for modal window (#89)

6527c48

* pull develop * new view for modal window nad its content Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

Remove border from main view (#93)

d9430e4

* pull develop * remove border Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

#29 Logika | Quiz - przeprowadzenie rozgrywki

8a5aceb

#74 Update for tablets view

fac753f

#75 Responsiveness for 1280 1920px

27990fc

#20 Add lightsaber timer

a1265e1

Fix for mobiles responsivity (#96)

32dfb2e

* pull develop * mobile responsivity fix Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

Fix for times names (#97)

bc28e37

* pull develop * time fixes * fix for tests Co-authored-by: Pawel <pawel.szambelan@flightscope.com.pl>

Fix for styles for 1280-1400px width screen (#98)

9e79b8d

Update README.md

20e4f96

README | Add presentation graphic

27f6ee1

HubertLegec reviewed Jan 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feedback & Code Review od Mentora Recenzenta (Hubert Legęć) #100

Feedback & Code Review od Mentora Recenzenta (Hubert Legęć) #100

Uh oh!

MateuszNaKodach commented Jan 19, 2021

Uh oh!

HubertLegec left a comment

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

HubertLegec Jan 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

		@@ -0,0 +1,12 @@
		@mixin brand-main-title {
		// display: inline-block;

		@@ -0,0 +1,38 @@
		const { fetchData } = require('../src/utils/fetchData');


		const result = getRandomIdFromArray(array);

		let hasDuplicate = result.some((val, i) => result.indexOf(val) !== i);

Feedback & Code Review od Mentora Recenzenta (Hubert Legęć) #100

Are you sure you want to change the base?

Feedback & Code Review od Mentora Recenzenta (Hubert Legęć) #100

Uh oh!

Conversation

MateuszNaKodach commented Jan 19, 2021

Uh oh!

HubertLegec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants