FreakingGrant / MI-449-js-localstorage

0 stars 0 forks source link

Project Feedback #1

Open FreakingGrant opened 4 years ago

FreakingGrant commented 4 years ago

Build a website that allows the user to switch between day and night color themes and remembers their choice

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

egillespie commented 4 years ago

Hi @FreakingGrant, I see your comment in the JavaScript file about trying to pass a function to an event listener by name. First, let me say that I like your day/night-theme toggling and it's working really well! I'll answer your question below and offer some additional feedback about the project. 😁

Passing a function to an event listener

The good news is that there is a way to pass a function by name to an event listener, and this is a great way to reuse a function, but the bad news is that you don't get to choose which arguments are passed to the function.

If you want to reuse a function that receives different arguments when called, like the one you've created:

function setTheme(theme) {
  document.body.setAttribute('class', theme)
  window.alert(theme + ' has been set')
}

You'll have to wrap that function in another function specific to the event listener. Here are some examples of what won't and will work:

// Will run the function, but theme will be undefined and won't work as expected
dayButton.addEventListener('click', setTheme)

// Calls setTheme('day-theme') right away and won't add an event listener
// because setTheme does not return a function
dayButton.addEventListener('click', setTheme('day-theme'))

// Will add an event listener that calls setTheme('day-theme') only when clicked
dayButton.addEventListener('click', function () {
  setTheme('day-theme')
})

See if you can use these examples to understand why your event listener wasn't working as expected and then update it to reuse the function in a way that will work. 👍

Add visit counter

Your theme toggle is working really well! I don't see a place where the number of page visits is tracked or shown.

Would you mind adding this feature to your site to meet requirement 6?


After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! 🚀

FreakingGrant commented 4 years ago

Changes were fixed and the visitor counter has been added!

egillespie commented 4 years ago

Nailed it. Nice work! 🤘 :shipit: