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

Mng 49 fill results modal #34

Merged
merged 7 commits into from
Jan 14, 2021
Merged

Mng 49 fill results modal #34

merged 7 commits into from
Jan 14, 2021

Conversation

AleksandraCyp
Copy link
Collaborator

No description provided.

@@ -76,7 +83,8 @@ export async function renderNextQuestion(generator) {
})
}
} else { // no more questions left
console.log("You WON!, but sorry, we still don't have any summary page"); // TODO create summary page redirection
fillResultsModal(GAME_HANDLER)
Copy link
Owner

@lukaszdutka lukaszdutka Jan 13, 2021

Choose a reason for hiding this comment

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

It should be GAME_HANDLER.getResults(timeLeft) - you want to base on object from getResults, not on the "private" properties of GAME_HANDLER

Comment on lines 18 to 22
if (questionItem.isCorrect === true) {
tableCell.querySelector('.tableWithResultsYourAnswer').style.border = '2px solid green';
} else {
tableCell.querySelector('.tableWithResultsYourAnswer').style.border = '2px solid red';
}
Copy link
Owner

Choose a reason for hiding this comment

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

Can be simplified to:

tableCell.querySelector('.tableWithResultsYourAnswer').style.border = questionItem.isCorrect ? '2px solid green' : '2px solid red';


const tableCell = document.createElement('tr');
tableCell.className = 'tableWithResultsQA';
if ((questionItem.question).substring(0, 3) !== 'http') {
Copy link
Owner

Choose a reason for hiding this comment

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

oh, it's cute but implementation shouldn't base on question starting with 'http' :P It doesn't tell much to other people. It would be better to pass "mode" to fillResultsModal function (or maybe even better - make it a property of gameHandlerResults) and decide based on mode.

const tableCell = document.createElement('tr');
tableCell.className = 'tableWithResultsQA';

if (mode.name === 'WHAT_DOES_THIS_POKEMON_LOOK_LIKE') {
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to reuse contants from modes file (like in PokemonService) - by this way you do not risk making a typo in string literal :)

mode === WHO_IS_THAT_POKEMON

Copy link
Owner

Choose a reason for hiding this comment

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

Think about: "what will happen if somebody changes literals and mode.name will return different, but really similar string?"

Then this code would not work :/

Copy link
Owner

@lukaszdutka lukaszdutka left a comment

Choose a reason for hiding this comment

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

One small thing and we are ready to go 😌

Copy link
Owner

@lukaszdutka lukaszdutka left a comment

Choose a reason for hiding this comment

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

Cool!!

@lukaszdutka lukaszdutka merged commit b7d8530 into dev Jan 14, 2021
@lukaszdutka lukaszdutka deleted the MNG-49-fillResultsModal branch January 14, 2021 12:11
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.

2 participants