jessicalam / MI-449-SS17-740-js-localstorage-wMUDce

project for MI449. demonstrated use of HTML5, javascript and CSS3, light/dark mode
https://jessicalam.github.io/MI-449-SS17-740-js-localstorage-wMUDce/
0 stars 0 forks source link

Project Feedback #1

Open jessicalam opened 7 years ago

jessicalam commented 7 years ago

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

is the page supposed to refresh every time the button is pushed? I didn't do that.

stuartpearman commented 7 years ago

I love the //FOR THE COUNTER ELEMENT comment, I encourage you to comment all your code like this (though you don't have to) 🙂

The only thing I would change is that on the first load of the page, you need to press the "Day / night mode" button twice before it changes to night mode. You can fix this by checking if window.localStorage.getItem('storage') returns anything and setting storage to 'day-mode' if not.

You can use an if statement to do this, or the || operator.

Let me know if you need a hint!

jessicalam commented 7 years ago

maybe this is fixed.... ? It seems like it is.

jessicalam commented 7 years ago

hi I think this is fixed. :)

stuartpearman commented 7 years ago

Hey Jessica, I think your code review got lost during the site outage, sorry about that, thanks for commenting again!

It seems not to be updated on the https://lamjess1-kapstpq2uw6ovwmudce.surge.sh/ link, did you run the surge command again? You can type this into your terminal:

surge . --domain lamjess1-kapstpq2uw6ovwmudce.surge.sh
jessicalam commented 7 years ago

I changed it! :)

stuartpearman commented 7 years ago

Hi Jessica, it seems to be broken 🙁

The culprit is this line here:

var storageTheme = window.localStorage.getItem('storage') || 'day-mode'

Everywhere else in your code, you're setting the 'theme' item in localStorage, here though you're setting 'storage'

Let me know if you have any questions

jessicalam commented 7 years ago

hopefully maybe this is okay now. :)

jessicalam commented 7 years ago

wait it says it was unable to publish. I'll ask you about this Tuesday

jessicalam commented 7 years ago

https://jessicalam.github.io/MI-449-SS17-740-js-localstorage-wMUDce/ Here is the link to the page because surge isn't working

stuartpearman commented 7 years ago

Looks good Jessica! :shipit: