nsuri1 / freecodecamp Goto Github PK

Your margins/paddings are off on the text box they are not accounting for the name of the author
https://goo.gl/photos/JNqwDLGHtFZ94rDE8 In case the image below doesn't work

• jQuery has a more readable way of assigning click event listeners that is the format $('selector').click( callbackFunction() )
• You have a global variable holding the current temperature which is OK (of course there is another way to hold the value that isn't global but its no worries for such a small project), but you keep manipulating its value whenever you click on Celsius versus Fahrenheit and rounding the output. There may exist a number where you click on the conversion and it rounds off the value making it imprecisely.
  • So in order to fix this I would keep the temperature stored in Fahrenheit as that seems to be how it is sent from the weather API, and if the user wants to see it in Celsius make the conversion and do not store the result just display the string in the GUI.
• The page takes a long time to load the weather info because you are calling it from document.ready it's pretty safe to assume that the initial page load will always be faster than making two ajax requests so I suggest calling it outside of the document.ready and just within the script itself. That should speed it up by about 500 ms or so
• I'm not a huge fan of switch cases (just a matter of preference) I say you should make an object literal to store the mappings and if the key does not exist in your predefined object that should be your default case
• so something like var mappings={clearDay:"clear-day.jpg",clearNight:"clear-night.jpg"}
• When you are showing and hiding the background using the animate function, jQuery has properties called show and hide respectively that use this function signature $('.background').show( timeInMilliseconds, callbackFunction ).hide( timeInMilliseconds , callbackFunction )
• Sorry to be annoying but it just makes it a little more clear what you are animating for