davisc91 / MI-449-js-localstorage

0 stars 0 forks source link

Project Feedback #1

Open davisc91 opened 4 years ago

davisc91 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:

I did not actually finish this project yet, I just need some help on how to get it working properly. Right now, I still can't figure out how to get localStorage to remember the theme choice. Also, I can't figure out how to make the page count appear on the page. In the console, it says "undefined is not an object" at line 36 and I'm not sure how to make it so that it isn't undefined anymore. I apologize if there is a lot that is wrong with my code, for some reason this isn't clicking with me right now!

Another question about project requirements: Does this website need to be multiple pages? Is satisfying the basic requirements okay, or do I need to do more? Just wondering because I saw it needs to be pushed to surge. Thank you!

egillespie commented 4 years ago

Hi @davisc91, you won't believe how close you are to having the theme code working! It surprised me at first that it didn't work — I think this is one of those things where a small little detail is easy to overlook and needs someone else to catch. 😅

I'll offer some feedback about your other questions below as well. You're doing really well on this project!

Theme loading

The reason the theme isn't loading comes down to a typo in line 11:

document.body.setAttribute('themeColor', themeColor)

Instead of setting the 'class' attribute on the body, you're setting 'themeColor'. If you replace the first parameter 'themeColor' with 'class', I think the theme will start loading as expected.

Page counter

It looks like you're on the right track with your page counter, too. I see lots of good code and it looks like things may have gotten mixed up when trying to work with the page element and variables at the same time.

Here's a step-by-step outline that may help untangle things:

  1. Remove .value from the end of line 3 - you're already setting .textContent on line 34 so you don't need the value of the element here
  2. Remove .value from the end of line 25 - parseInt will give you a number or NaN
  3. Inside your counterFunction, do this:
    1. If visitNumber is not a number, set it to 1
    2. Otherwise, set visitNumber to visitNumber + 1
    3. Save visitNumber to localStorage
    4. Return the value of visitNumber

If this outline doesn't make sense or you get stuck, let me know and I'll offer tips in a different way.

Only one page is needed

I recommend keeping this project as small as possible. Save the extra pages and design-work for the CSS projects. 😉


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

davisc91 commented 4 years ago

I fixed those things and I changed it to one button instead of two!

egillespie commented 4 years ago

Great work with the page counter, and the single-button theme toggle is a nice touch as well. I really like how this project came together! Awesome work, I don't have any other recommendations! 🎉 :shipit: