englecal / MI-449-SS18-740-js-localstorage-wMUDce

0 stars 0 forks source link

Project Feedback #1

Open englecal opened 6 years ago

englecal commented 6 years ago

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

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

egillespie commented 6 years ago

Great job on this project, @englecal! 💪

Your page works really well. My only suggestions for improvement are focused on code clarity. Would you mind trying these out?

1. Separate the theme and page-count code

Would you mind moving your lines of code around so that all of the code to manage the theme is in one big clump and all of the page-count code is also in one sequential clump?

Organizing code to keep bits of functionality close together within a file is a great way to make your code easier to read and to make your code modular — meaning if you wanted to split the theme and page-count code into two separate files, you could copy and paste a single block of code instead of hunting down a bunch of lines scattered across the file.

For example, your first two lines are related to the theme functionality, then the next eight statements do some page-count logic, then there's some more theme code.

2. Introduce a function to set the theme

Your theme toggler has an if/else with very similar functionality in each block:

  if (document.body.getAttribute('class') === 'day-theme') {
    // if body them is day-theme, sets body to class night-theme
    document.body.setAttribute('class', 'night-theme')
    // sets localStorage to night-theme
    window.localStorage.setItem('theme', 'night-theme')
  } else {
    // if body them is day-theme, sets body to class day-theme
    document.body.setAttribute('class', 'day-theme')
    // sets localStorage to day-theme
    window.localStorage.setItem('theme', 'day-theme')
  }

Would you mind introducing a function that accepts the theme as an argument and sets that value on both the body element's class attribute and in local storage?


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! 🤘

englecal commented 6 years ago

ready for another look

egillespie commented 6 years ago

The function looks great! 😻 Would you mind taking on my first request, "Separate the theme and page-count code"?

englecal commented 6 years ago

ready for another look

egillespie commented 6 years ago

Beautiful! Thanks! 🤜💥🤛 :shipit: