Project Feedback

[amanuel1995]

@stuartpearman Can you take a look at this? It's hosted here and meets the following criteria:

• [x] Includes a button that when clicked, will toggle the website between day and night themes
• [x] When a user changes the theme, their choice is remembered in localStorage
• [x] The class on the body element is always up-to-date with the current theme (either day-theme or night-theme) -- you can use document.body.setAttribute('class', 'some-class-name') to set the class on the body
• [x] When the body has the class day-theme, the background of the website is light-colored, with a darker color for the text
• [x] When the body has the class night-theme, the background of the website is a very dark color, with a lighter color for the text
• [x] Somewhere on the page, the user is told how many times they've visited the website (starting with 1 the first time)

[stuartpearman]

Hi Amanuel,

Overall, everything is great and works fine. The only things that could be better are a few areas where your code is redundant or formatted slightly wrong.

1. I see some ; semicolons after your Dim and Bright functions. We're using JavaScript Standard to format, so these aren't needed
2. In your if (themeChoice === null) statement (line 23), for both true and false cases, you add event listeners to the two buttons. You could instead add these event listeners outside of the if statement and clean things up a bit. You could rework the if statement to check if (themeChoice !== null), adding the existing class to the body element if the condition is met.

And here's some bonus learning, I don't expect you to get this on your own but thought it would be helpful to include. This block of code:

if (count != null) {
  var updateCount = parseInt(window.localStorage.getItem('count'))
  updateCount++
  window.localStorage.setItem('count', updateCount) // store updated value
  visitCount.textContent = 'You have visited this page: ' + updateCount + ' times.'
} else {
  count = 1
  updateCount = 1
  visitCount.textContent = 'You have visited this page: ' + updateCount + ' times.'
  window.localStorage.setItem('count', updateCount) // store updated value
}

could be refactored to:

if (count != null) {
  count = parseInt(count) + 1
} else {
  count = 1
}

visitCount.textContent = 'You have visited this page: ' + updateCount + ' times.'
window.localStorage.setItem('count', updateCount) // store updated value

or better yet, when you declare the count variable, you could add an || operator to do the same thing that if (count != null) does:

var count = window.localStorage.getItem('count') || 1

The || operator will return the first truthy value, so if window.localStorage.getItem('count') is null (which is falsey), it will return 1.

Great job on this! I'm super happy with your quality of work here Amanuel :+1: :sunglasses:

[amanuel1995]

Thank you for your kind words and smart suggestions.

1. I thought semicolons are only needed after function declarations and the js-linter did not show me any warning or error.
2. About simplifying code: that's smart man! I didn't think about that. I like simplified and clean code. I have taken out the redundant lines and simplified wherever it was possible. I have pushed the changes to this repo and updated the live site as well. Let me know if I am done or there's something more I could do! 😄

[stuartpearman]

Looks great! If you want to see a cool visualization of those changes, git does just that in the diff. You'll see a lot of red, signifying all the lines you were able to get rid of :+1:

You can click on your commit history (look for the button pictured below) from your repository to find a summary of all your commits:

and clicking into any one of your commits will show you the changes or diff from that commit.

Thought that might interest you. Anywho, nice work! :shipit:

[stuartpearman]

Oh and to answer your question, semicolons are pretty much not needed in javascript these days, although you will still use them in many other programming languages.