Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feedback branch for nowakprojects #68

Open
wants to merge 70 commits into
base: feedback
Choose a base branch
from
Open

Feedback branch for nowakprojects #68

wants to merge 70 commits into from

Conversation

lukaszdutka
Copy link
Owner

No description provided.

daria305 and others added 30 commits December 30, 2020 22:20
this is my change, it is cool. Margin should be 10
this MUST be 20, change on dev branch
commit to dev
this is my super change;

Paired with <somebody>
* Add a starting page HTML template

* change template HTML for the starting page into a js file
* MNG-15 Add API urls, start tests and function for pokemons.js

* Change url end

Co-authored-by: Łukasz Dutka <lukaszdutka2@gmail.com>

* Make function async

Co-authored-by: Łukasz Dutka <lukaszdutka2@gmail.com>

* Correct url end

* MNG-15 Recreate test function for getPokemonById to follow the rule given, when, then

* MNG-15 Write first version of getPokemonById

* MNG-15 Fix test async function handling with beforeAll

* MNG-15 Add test for get types by id

* MNG-15 Create getTypeById function

* Refactoring tests and getPokemonById to be more readable.

Remove nested describe functions,
not use destructuring with pokemon data

Co-authored-by: Łukasz Dutka <lukaszdutka2@gmail.com>
Kamil - Add my name
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>
* Update img and my name

* MNG-22 add IMG

* MNG-22 Rename IMG
* Update img and my name

* MNG-22 add IMG

* MNG-22 Rename IMG

* Add SVG icons
* Good but for corrections

* Now better

* Responsiveness + little corrections

* Centering some elements + correcting "Quiz"

* Comments

* Bolder question mark

* Merge branch 'dev' into MNG-12-Style-out-StartingPage-(initially)

* Correction

Co-authored-by: Łukasz Dutka <lukaszdutka2@gmail.com>
… is not… (#16)

* Add a help screen, style it (has to be polished, the scrollbar is not styled). Add help screen HTML the the page, add an event listener for the help screen and a reusable function showAPopUpScreen (can be used to display hall of fame)

* Remove event listener from the exit button every time it is clicked, correct spelling mistakes
* Add randomPokemonId method

* Add methods - answers and correctAnswer

* Add getNextQuestion method

* Add checkAnswer method

* Remove App import

* Add tests for randomPokemonId, answers and correctAnswer methods

* Add test for getNextQuestion method

* Change name of file and class to QuestionService

* Modify questionService.js name

* Fix for bug in correctAnswerId variable

* Modify getNextQuestion to resolve promise from getPokemonById

* Add promise.all but still sth is wrong

* Add test for getNextQuestion method

* Add object scheme to QuestionService

* Add test for checkAnswer method

* Add correctAnswerIndex to class QuestionService

* Remove comments

* Modify import in test file

* Modify import in test file

* Update test for checkAnswer method

* Modify modes

* Fix mode typo

* Fix test typo

* Add mode parameter to getNextQuestion method
* print the name of clicked button

* no possibility to click on the button while a hel screen is active

* Correction

Co-authored-by: AgataLudwiczynska <74932979+AgataLudwiczynska@users.noreply.github.com>
…sts / font secure / add icon / change website title

* Add a randomNumberInRange function and tests

* Change page title, add an icon, change http to https

* Change raundomNumberInRange.js, change the relative tests

* change error messages, if statements, min>max logic, remove parenthesis
* Add instructions

* Update src/app/addHelpScreenTemplate.js

Co-authored-by: Łukasz Dutka <lukaszdutka2@gmail.com>
* MNG-11 Add quiz template to index.html and create js file to render this template

* Update quiz template, add template for quiz answer

* MNG-11 Create renderQuizPage and functions for question/mode-title updates

Use templates provided in index.html to render the page.
Use service question syntax for getting the question.

* MNG-11 Update for quizPage, add question/answers items functions

Still needed to correct error in template reading

* MNG-11 Fiish rendering quiz questions

* MNG-11 All finished before user selects the answer

* Fix styles, to avoid cascading main page layout to the quizPage

* Fix styles to avoid cascading main page styles with quizApp id - after rebase

* MNG-11 Functions for selecting answers and checking the correctness

* MNG-11 Questions are now changing, collect styles in appSettings

* Update src/app/App.js change to arrow

Co-authored-by: Łukasz Dutka <lukaszdutka2@gmail.com>

* Small fix  for mode variable

Co-authored-by: Łukasz Dutka <lukaszdutka2@gmail.com>
* Initial commit

* Rename getNextQuestion method

* Add class QestionGenerator and test

* Modify class QestionGenerator

* Add test for askedQuestionsCount property

* Remove correctAnswerIndex property

* Use aksedQuestion array

* Correct which ids can repeat
* Add shuffleAnswers function and relative tests

* Correct a spelling mistake

* Import shuffleAnswers to questionService

* Import shuffleAnswers to questionService - fix
…x.html. Remove the script rendering help modal, put it in showStartingPage.js (#24)
AgataLudwiczynska and others added 27 commits January 15, 2021 19:46
* Autocompletion off + button "Play"

* Play button disabled by default
chenged grid to flex
height for answer img
some random font property for pokemon name (quiz-question class)
Now it display answers (img too) in one column for small width screens
* Add initial styling to time bar

* Add z-index

* Add border-radius to quiz bar

* Change time-bar color

* Fix progress bar width

* Add padding to timerLabel and question-counter

* Add z-index to modal

* Change progress bar width

* Center question text

* Fix width for timerLabel

* Change slowly timer bar color to red
* MNG-35 styline quiz page, add img background under pokemon

* MNG-35 Style out mode1 and 2, add background under the pokemon

* MNG-35 Change the progress bar style
* MNG - Add 3rd Game Mode

* MNG-58 and MNG-61

* update

* Issue with style
… modal (#51)

* Remove console-logs

* Change color of buttonWithText on hover

* Add margin to help modal
* Add pointer, change results modal, add custom scrollbars

* Fix pointer
* MNG-65 Fix onclick event on quizPage

* Add hover styles to the quiz Page
changed: img tag to svg in html file
added animation to svg
* Style button for first mode

* Style button for default mode

* Add hover for mode buttons
* MNG-69 Add total num of questions on the quiz page

* MNG-69 Capitalize pokemon names

* MNG-69 Move capitalize name feature from css to QuestionService

* MNG-69 Change to map

* MNG-69 Refactore capitalize

* Refactor again
* MNG-68 Update README.md file

Updated project description and images

* Typos
Co-authored-by: AgataLudwiczynska <74932979+AgataLudwiczynska@users.noreply.github.com>
@@ -1,2 +1,2 @@
POKEMON_API_BASE_URL =
POKEMON_API_BASE_URL = "https://pokeapi.co/api/v2"
Copy link

@MateuszNaKodach MateuszNaKodach Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus za użycie zmiennej środowiskowej.

const POKEMON_API_BASE_URL = process.env.POKEMON_API_BASE_URL || "https://swapi.dev/api";
const QUIZ_MAX_TIME = process.env.QUIZ_MAX_TIME_SECONDS ? process.env.QUIZ_MAX_TIME_SECONDS * ONE_SECOND_MILLIS : 120 * ONE_SECOND_MILLIS;
const POKEMON_API_BASE_URL = process.env.POKEMON_API_BASE_URL || "https://pokeapi.co/api/v2";
const QUIZ_MAX_TIME = process.env.QUIZ_MAX_TIME_SECONDS ? process.env.QUIZ_MAX_TIME_SECONDS : 120;

window.onload = () => App({options: {pokemonApiBaseUrl: POKEMON_API_BASE_URL, quizMaxTime: QUIZ_MAX_TIME}})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tutaj obiekt options niepotrzebnie ma przekazywany parametr pokemonApiBaseUrl, bo nie jest on potem używany.

Copy link

@MateuszNaKodach MateuszNaKodach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jakieś wskazówki Wam zostawiłem, ale ogólnie jak na 1 projekt i to z zespołem, który jeszcze wcześniej ze sobą nie pracował to świeta robota!

Oby dalej było jeszcze tylko lepiej.
Wasza aplikacja jak na razie podobała mi się najbardziej ze wszystkich! :)

Powodzenia w dalszej drodze do web developera!

Ocena implementacji: 9/10

TOTAL_NUM_OF_QUESTIONS,
} from "./appSettings.js"

import {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brakuje konsekwencji i wspólnych ustawień formatowania.


const startTimer = (bar, timerDuration) => {
// durationTime time in seconds, can be changed freely 120 -> 120 seconds = 2 minutes
durationTime = timerDuration

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raczej niepotrzebne przypisanie jednej zmiennej do drugiej:
durationTime = timerDuration

}
};

export async function getTypeById(id) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super, że radzicie sobie z Promisami i zapisem async/await, co tutaj widać! :)


describe("Test pokemon API to get pokemon types", () => {

it("Given the type id is 12 when asking for pokemon data, should return id and name of the type", async () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bardzo ładny opis testów. Normalnie jakby widział swoje :)
Plus za podział na sekcje Given-When-Then.

types: [{
id: 12,
type: "grass"
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kurde, tyle razy grałem w Pokemony, a jakoś nie zwróciłem uwagi, że Bulbasaur jest też typu Poison! Fajną wiedzę domenową można wyczytać z testu :)

return result;
}

capitalize(str) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeśli jakaś funkcja jest jedynie do użycia wewnątrz tej klasy to warto użyć konwencji i nazwać ją _capitalize co w JS oznacza takie "ułomne private".

}
}

// Subtruct time left from total time

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dałoby radę zmienić nazwę metody na bardziej opisową i wtedy wywalić ten komentarz :) ?
Komenatrze mogą się dezaktualizować i mówić nie to co naprawdę robi kod pod nimi.


appScreen.innerHTML += pikachuSvgImage.innerHTML;

// add leaderboard modal

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Można to zastąpić po prostu metodą addLeaderboardModal i komentarz jest zbędny.

@@ -0,0 +1,269 @@
import {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dobrze byłoby to rozbić na pliki dla poszczególnym komponentów widoku. Ciężko się odnaleźć w tak długich plikach.

@@ -1,3 +1,876 @@
@import url('https://fonts.googleapis.com/css2?family=Raleway:wght@600;900&family=Ranchers&family=Secular+One&display=swap');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fajnie by było podzielić ten CSS na mniejsze (cięzko np. znaleźć style odpowiedzialne jedynie za ranking) i przyjąć jakąś konwencję nazywania klas itp.
Co powiecie na notację BEM?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants