add hints (#153) #168
add hints (#153) #168
Conversation
Pull Request Test Coverage Report for Build 372
💛 - Coveralls |
|
I will resolve the conflict |
|
@ollelauribostrom conflicts has been resolved |
|
@gizemkeser Nice job, I'll have a look at this tomorrow 🙂 |
There was a problem hiding this comment.
Great job 👍 I added a few comments about some things that needs to be looked at before merging this. The render framework does not handle partial updates very well and having the hint placed inside of the Rebus component will cause the entire Rebus component to update when the hint appears. Could you extract it to its own component and either make it a child of the Rebus component or just render it separately from app.js?
src/js/components/rebus.js
Outdated
| <div class="rebus__header"> | ||
| <span>${current + 1}/${rebuses.length}</span> | ||
| </div> | ||
| <span class="rebus__symbols">${rebus.symbols.join(' ')}</span> | ||
| <div class="rebus__words"> | ||
| <children> | ||
| </div> | ||
| ${ |
There was a problem hiding this comment.
I think it would be a good idea to break out the hint to its own component in a separate file. The render framework currently does not handle partial updates very well, and this will cause the entire Rebus component to update when the hint appears.
There was a problem hiding this comment.
@ollelauribostrom thanks for your feedback, it's done.
| @@ -0,0 +1,2 @@ | |||
| export const INCORRECT_ANSWER_MAX_COUNT = 3; | |||
There was a problem hiding this comment.
Move this to the same file as the rebus component
|
Nice job, thank you 🙂 |
Associated Issue: #153
Summary of Changes