Add progress bar and score percentage as discussed in #102

[rreubenreyes]

Small issue:

This is what the progress bar looks like right now. There's not anything behaviorally wrong with it, but I just noticed that when zoomed out, the red segment appears to be slightly lower than the green segment (it's not, if you check the page source). I'm not sure if I should bother trying to fix this as it's mostly a design issue right now, but if anyone has any suggestions I'm all ears!

[codecov-io]

Codecov Report

Merging #104 into master will decrease coverage by 0.92%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   89.26%   88.33%   -0.93%     
==========================================
  Files          28       30       +2     
  Lines         149      180      +31     
  Branches       13       17       +4     
==========================================
+ Hits          133      159      +26     
- Misses         15       20       +5     
  Partials        1        1

[rreubenreyes]

(Lines 75-79) when implementing the updated score object, I ran into this weird bug(?). My goal was to remove newScore altogether and just contain the entire "newScore" object as an argument to updateScore() (line 92) without an additional variable declaration. However, this (lines 75 - 79) happens.

The app behaves as expected with the changes as they are now, but would just like to know why this happens and if I can indeed minify the code a little as I described.

[austintackaberry]

I'm not sure I totally understand your comment, but setting newScore = score isn't creating a new object. It is pointing newScore at score. So what you are really doing is mutating score. When you do newScore.books = score.books.map. . ., it is the same thing as score.books = score.books.map . . . This would be a good use case for the spread operator.

EDIT: Ok, I figured out the issue. In App.js, we aren't sending this.state.score to the Sidebar. We are sending score. So we can only update it if we mutate it. We need to change it to this.state.score and it works.

[austintackaberry]

Here is a way to update the score immutably:

const newScore = {...score, books: score.books.map((book, index) => {
  if (index === bookId) {
    return {
      ...book,
      correct: book.correct + scoreDiff.correct,
      incorrect: book.incorrect + scoreDiff.incorrect,
      chapters: book.chapters.map((chapter, index) => {
        if (index === chapterId) {
          return {
            ...chapter,
            questions: chapter.questions.map((question, index) => {
              if (questionIndex === index) {
                return {
                  answered: true || question.answered,
                  correct: isCorrect,
                };
              }
              return question
            })
          }
        }
        return chapter
      })
    }
  }
  return book
})}

In order to use the spread operator we need to add:
plugins: ['transform-object-rest-spread'], to the babelrc

[austintackaberry]

Just thinking out loud...I feel like the best way to structure score would be to not store book.correct or book.incorrect. Also, for each question, instead of {answered, correct}, we just have {status: "correct" }, {status: "incorrect" }, or {status: "unanswered" }

Then we should just use functions to calculate everything we need to get out of the data structure. This doesn't have to be a part of this PR...just a thought I had.

[austintackaberry]

Dude, this looks awesome! Couple comments about using the spread operator and fixing a bug in App.js, but nothing major

[austintackaberry]

I'm not sure I totally understand your comment, but setting newScore = score isn't creating a new object. It is pointing newScore at score. So what you are really doing is mutating score. When you do newScore.books = score.books.map. . ., it is the same thing as score.books = score.books.map . . . This would be a good use case for the spread operator.

EDIT: Ok, I figured out the issue. In App.js, we aren't sending this.state.score to the Sidebar. We are sending score. So we can only update it if we mutate it. We need to change it to this.state.score and it works.

[austintackaberry]

Here is a way to update the score immutably:

const newScore = {...score, books: score.books.map((book, index) => {
  if (index === bookId) {
    return {
      ...book,
      correct: book.correct + scoreDiff.correct,
      incorrect: book.incorrect + scoreDiff.incorrect,
      chapters: book.chapters.map((chapter, index) => {
        if (index === chapterId) {
          return {
            ...chapter,
            questions: chapter.questions.map((question, index) => {
              if (questionIndex === index) {
                return {
                  answered: true || question.answered,
                  correct: isCorrect,
                };
              }
              return question
            })
          }
        }
        return chapter
      })
    }
  }
  return book
})}

In order to use the spread operator we need to add:
plugins: ['transform-object-rest-spread'], to the babelrc

[austintackaberry]

Just thinking out loud...I feel like the best way to structure score would be to not store book.correct or book.incorrect. Also, for each question, instead of {answered, correct}, we just have {status: "correct" }, {status: "incorrect" }, or {status: "unanswered" }

Then we should just use functions to calculate everything we need to get out of the data structure. This doesn't have to be a part of this PR...just a thought I had.

[rreubenreyes]

Thanks for the feedback! I went ahead and implemented your suggestions and everything still seems fine. But suddenly, this:

It seems like this has happened to others before for no particular reason. I'm still looking into it, I'll push the changes once I find one.

[austintackaberry]

I think you need to stage your changes to .babelrc.js, package.json, and package-lock.json

[rreubenreyes]

Doesn't seem to be it:

[austintackaberry]

Hmmm...I made my own test branch where I added spread operator and added that line to the .babelrc and committed it, and I didn't have a problem. If you're all out of ideas, maybe just bypass the git hook, so that others can take a look and help debug?

[rreubenreyes]

Just pushed the changes while bypassing the linter problem. Still trying to figure out what's causing this.

[austintackaberry]

I believe the Sidebar book scores are showing (undefined / undefined). Probably the best way to fix this is to just have a helper function and use it in Sidebar.js to calculate the score for each book.

[nikrb]

just pulled this branch to look at the issues, but it seems to work for me. Added a test for spread operator and that too seems fine.
Are you still experiencing issues with this pr?

[rreubenreyes]

I think we're good. I'm going to re-add support for book scores in Sidebar.js this afternoon when I'm out of work 🙂

[rreubenreyes]

Just pushed the changes. Cleaned up the Sidebar.js tests a little to remove the dummy variable test suite, since I added the helper function that depends on the real data now. Played around to make sure nothing is wrong, and I haven't caught anything on my end. Let me know if we're good!

P.S. I'm liking the new design 😄

[austintackaberry]

Looks good! Only thing is that the book sidebar isn't updating for me.

EDIT: Looks like score isn't ever making its way into getBookScores

[rreubenreyes]

Noted, and found a few more bugs as well. Will have a resolution soon.

[austintackaberry]

Looks good to me! Thanks for being open to feedback! 🙂