Feedback & Code Review od Mentora Recenzenta (Jan Owsiany) by Nefariusek · Pull Request #103 · Nefariusek/CodersCamp2021.Project.JavaScript

Nefariusek · 2022-01-05T17:39:08Z

No description provided.

Feature/issue 21

Feature issue#14

…nally styled

…zSettings

Feature/issue 27

[hotfix] Making the adoption bubble behave like the fact bubble

Enhancement/issue 73

Feature/issue 62

Fixing paths and urls for the production version

Fixing paths for the production version

added responsive view to all screens

Release 1.0

janowsiany

Unfortunately, I couldn't go through everything deeply, I focused on JavaScript part and just looked briefly on CSS
The general comment is i like when the project uses a consistent naming convention, i did not find it here 😅
On the other congrats on linking it all together, i see a huge effort here 🍝, as for the first project the scope of it is impressive 👏
Also, the code in multiple places had great quality and looked very professionally 🎖️

janowsiany · 2022-01-05T19:56:42Z

src/api/FactsController.js

+    const factJson = await factResponse.json();
+    const imgJson = await imgResponse.json();


Suggested change

const factJson = await factResponse.json();

const imgJson = await imgResponse.json();

const [factJson, imgJson] = await Promise.all([factResponse.json(), imgResponse.json()]);

janowsiany · 2022-01-05T19:59:12Z

src/api/FactsController.js

+    const factResult = {
+      fact: factJson.text,
+      imageUrl: imgJson[0].url,
+    };
+    return factResult;


Suggested change

const factResult = {

fact: factJson.text,

imageUrl: imgJson[0].url,

};

return factResult;

return {

fact: factJson.text,

imageUrl: imgJson[0].url,

};

i like to skip variable declaration if it is used only for return

janowsiany · 2022-01-05T20:00:40Z

src/api/FactsController.js

+  const randomNumber = randomIntFromInterval(0, animalTypes.length - 1);
+  const randomAnimalType = animalTypes[randomNumber];


Suggested change

const randomNumber = randomIntFromInterval(0, animalTypes.length - 1);

const randomAnimalType = animalTypes[randomNumber];

const randomInt = randomIntFromInterval(0, animalTypes.length - 1);

const randomAnimalType = animalTypes[randomInt];

when there is a name in getter function then i like to have the corresponding name for variable, using consistent vocabulary is IMHO important

janowsiany · 2022-01-05T20:02:53Z

src/api/getQuizQuestions.js

+const questionsApis = {
+  DOGS: { url: 'https://api.thedogapi.com/v1/breeds', api_key: '62b8cc9a-2c13-4eec-abe2-80703eabb0a6' },
+  CATS: { url: 'https://api.thecatapi.com/v1/breeds', api_key: '2d4cf1ee-1883-474f-80ab-f931262fd79b' },
+};


i do not like mixing cases and naming conventions for variables

janowsiany · 2022-01-05T20:03:50Z

src/api/getQuizQuestions.js

+  DOGS: { url: 'https://api.thedogapi.com/v1/breeds', api_key: '62b8cc9a-2c13-4eec-abe2-80703eabb0a6' },
+  CATS: { url: 'https://api.thecatapi.com/v1/breeds', api_key: '2d4cf1ee-1883-474f-80ab-f931262fd79b' },


I think it is important to tell the team that putting api_key in git repositories is antipattern and should never happen

moved to .env

janowsiany · 2022-01-05T21:07:35Z

src/components/timer/quiz-timer.js

+  clearInterval(timer);
+}
+
+function timerDigits(time_value) {


inconsistent variable naming time_value

changed to timeValue

janowsiany · 2022-01-05T21:21:04Z

src/views/quiz-settings/quiz-settings.js

+import '../../style.css';
+import Button from '../../components/Button/Button';
+
+class QuizSettings {


to be honest i do not like the way it is implemented with static methods, seems overengineered for me

janowsiany · 2022-01-05T21:34:21Z

src/components/timer/quiz-timer.js

+  }, 1000);
+}
+
+export function stopTimer() {


It is not used

janowsiany · 2022-01-05T21:40:33Z

src/views/QuizView/quizView.js

+  const time = timerMinutes * 60 + timerSeconds;
+  return time;


Suggested change

const time = timerMinutes * 60 + timerSeconds;

return time;

return timerMinutes * 60 + timerSeconds;

janowsiany · 2022-01-05T21:42:29Z

src/views/QuizView/quizView.js

+  timerMinutes = document.getElementById('timer-minutes').innerText;
+  timerSeconds = document.getElementById('timer-seconds').innerText;


I do not like that the state(minutes and seconds) is shared through HTML element 😞

used export/import instead

Fix/feedback

Urszuja · 2022-05-25T18:40:55Z

changes introduced.

JSDoc component documentation

add time manager as singleton class

Urszuja and others added 30 commits December 8, 2021 22:15

add gestAnswers - function for answers shuffling

6a8a32c

added Score Page without funtionality and styled it

f9f44a1

plus js file

43454aa

Merge pull request #31 from Nefariusek/feature/issue-21

7d6b8c3

Feature/issue 21

fixed callbacks and added nickname validation

c8ce0c7

Merge branch 'develop' into feature-issue#14

05d9540

Merge pull request #25 from Nefariusek/feature-issue#14

b886488

Feature issue#14

initialize router

eb249fe

Merge branch 'develop' into feature/issue-30 added functionallity, fi…

e9e801a

…nally styled

changed return div to .append

af22b05

fix router paths

fc7f0be

clean router.js file

52493fc

add fetch cats

ee50783

add enum + switch - providing cat or dog array

343e10f

Page UI first version

4fca1d4

Page UI secodn version

cb384c5

add prepared questions data set for both cats and dogs

4232c57

fix problems

a399070

fixed css added pre vresion of routing to buttons in ScorePage an Qui…

6977415

…zSettings

prettier

d9e3b8a

add rediectToMainPage

d1eac2e

Merge pull request #32 from Nefariusek/feature/issue-27

4ae3323

Feature/issue 27

code rename

6313194

add logic to scorepage

0c9b6b4

code cleaning part 1

601530c

fixed naming

d176206

Merge branch 'develop' into feature/issue-30

9e3379b

add new function that sort scores in correct order

05d91a6

clean up the code

86df01c

Merge branch 'develop' into feature/issue-28

5910911

Urszuja and others added 17 commits January 3, 2022 21:40

basic styles

20ec79e

cleanup

f0184b2

better image proportions in bubbles

1f092cb

Merge branch 'develop' into hotfix/issue-79

8bf7591

added responsive view

8d0fdef

Merge pull request #92 from Nefariusek/hotfix/issue-79

0c45c60

[hotfix] Making the adoption bubble behave like the fact bubble

Merge pull request #94 from Nefariusek/enhancement/issue-73

aaa5aa3

Enhancement/issue 73

Merge branch 'develop' into feature/issue-62

7e0095e

Merge branch 'develop' into feature/issue-62

3566d4c

Merge pull request #93 from Nefariusek/feature/issue-62

39899a2

Feature/issue 62

Fixing paths and urls for the production version

a0c8123

Merge pull request #98 from Nefariusek/bug/issue-97

09d6b8a

Fixing paths and urls for the production version

Fixing paths for the production version

4388a73

Merge pull request #99 from Nefariusek/hotfix/issue-97

32cf160

Fixing paths for the production version

added responsive view to all screens

237ed09

Merge pull request #100 from Nefariusek/bug/issue-88

a8a5301

added responsive view to all screens

Merge pull request #101 from Nefariusek/develop

e7f58d6

Release 1.0

Nefariusek changed the title ~~Feedback & Code Review od Mentora Recenzenta (Mateusz Ossoliński)~~ Feedback & Code Review od Mentora Recenzenta (Jan Owsiany) Jan 5, 2022

janowsiany reviewed Jan 5, 2022

View reviewed changes

Urszuja added 5 commits May 8, 2022 22:11

env file added

a70b04f

services dist created

4fa20e7

add set intead of array for retrieving false answers

5421747

merge filters into one

8d104c2

Merge pull request #104 from Nefariusek/fix/feedback

cb10564

Fix/feedback

Nefariusek requested a review from Urszuja May 25, 2022 18:42

mngweb and others added 4 commits May 25, 2022 21:06

JSDoc component documentation

2820c9e

Merge pull request #105 from Nefariusek/hotfix/jsdoc

913a9cc

JSDoc component documentation

add time manager as singleton class

6a110b8

Merge pull request #106 from Nefariusek/fix/feedback

0b4e9c6

add time manager as singleton class

		const factJson = await factResponse.json();
		const imgJson = await imgResponse.json();

	const factJson = await factResponse.json();
	const imgJson = await imgResponse.json();
	const [factJson, imgJson] = await Promise.all([factResponse.json(), imgResponse.json()]);

		const randomNumber = randomIntFromInterval(0, animalTypes.length - 1);
		const randomAnimalType = animalTypes[randomNumber];

		DOGS: { url: 'https://api.thedogapi.com/v1/breeds', api_key: '62b8cc9a-2c13-4eec-abe2-80703eabb0a6' },
		CATS: { url: 'https://api.thecatapi.com/v1/breeds', api_key: '2d4cf1ee-1883-474f-80ab-f931262fd79b' },

	const time = timerMinutes * 60 + timerSeconds;
	return time;
	return timerMinutes * 60 + timerSeconds;

		timerMinutes = document.getElementById('timer-minutes').innerText;
		timerSeconds = document.getElementById('timer-seconds').innerText;

Conversation

Nefariusek commented Jan 5, 2022

Uh oh!

janowsiany 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!

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!

Urszuja commented May 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants