Conversation
…s 3-4 in progress, not done yet
LocationIQ API and OpenWeather API implemented and work with proxy. Waves 3 and 4 done
…led header and Rename city button, rearranged the code for better readability (old one showing all the changes I made to code itself left below as comment)
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
There was a problem hiding this comment.
🔎 Weather Report was a plain JS project, so a package.json file isn't necessary, though it could be added as a means of getting eslint checking!
There was a problem hiding this comment.
Consider including your wireframes in the repo. If you planned out the site with a quick sketch, even taking a quick picture and including that can be helpful. Or if it's authored in a different (online) tool, consider adding a document with a link to it, or including it in your README.
| <script src="https://unpkg.com/axios/dist/axios.min.js"></script> | ||
| <script src="src/index.js" defer></script> |
There was a problem hiding this comment.
Note that originally, the index.js script link was at the end of the body, and the directions were to include the axios script just above the index.js script. This was fine when the script was at the end of the body, as both scripts will be loaded after the body has been parsed.
Here, you've moved the script to the head. Using defer allows the browser to load index.js concurrently with the rest of the body, while still waiting to run until after the page has been processed. But we should then also defer the axios script, since it will block the browser while it's loading. defer still ensures the relative order execution of scripts, so even if your index.js made use of axios at a global level, this would still be fine.
| // ⬆️⬇️ TEMP CONTROLS | ||
| // ================================ | ||
| const updateTemp = (delta) => { | ||
| const tempDisplay = document.getElementById('temperature'); |
There was a problem hiding this comment.
Rather than fetching elements anywhere I need them, I like to make some variables to hold each element I need, and fetch them all together at page load. Then I can just refer to the stored variables later when I need access to the elements rather than needing to retype string ids in multiple places, which can lead to typos and maintainability challenges.
| // ================================ | ||
| const updateTemp = (delta) => { | ||
| const tempDisplay = document.getElementById('temperature'); | ||
| let currentTemp = parseInt(tempDisplay.textContent, 10); |
There was a problem hiding this comment.
Rather than reading the temperature value back out of the UI, requiring parsing, we could make a variable that holds the temperature as a number. Then when we need to update the temperature, we can work directly with the numeric value, then "publish" it out to the UI element. It's generally more maintainable to hold data in actual variables, and treat the UI as a one-way output source.
There was a problem hiding this comment.
Nit: prefer const wherever possible.
| <option value="sunny">Sunny</option> | ||
| <option value="cloudy">Cloudy</option> | ||
| <option value="rainy">Rainy</option> | ||
| <option value="snowy">Snowy</option> |
There was a problem hiding this comment.
🔎 Rather than explicitly coding the sky options here, consider how we might write code to populate the options with values that we know our logic supports. Manually writing the options here introduces the possibility that there could be a mismatch between the options we wrote, and the options our logic is written to handle.
| increaseTempBtn.addEventListener('click', increaseTemp); | ||
| decreaseTempBtn.addEventListener('click', decreaseTemp); | ||
| //sky selection change (Wave5) | ||
| skySelect.addEventListener('change', updateSkyDisplay); |
There was a problem hiding this comment.
👍 Registering for the change event will send us a notification each time the selected value changes. For consistency, we sometimes need to manually trigger any logic associated with the input to ensure that any dependent UI matches the default displayed selection.
You addressed this with the additional initializeSkyDisplay function, which is called at page load, but could updateSkyDisplay handle this scenario?
| const skyDisplay = document.getElementById('sky-display'); | ||
| const selectedSky = skySelect.value; | ||
|
|
||
| skyDisplay.textContent = SKY_OPTIONS[selectedSky]; |
There was a problem hiding this comment.
👍 Nice use of an object to store the value to pattern mapping. Since the sky settings are a discrete set of options, an object is a handy way to look them up.
|
|
||
| //button clicks - city | ||
| // cityRenameBtn.addEventListener('click', renameCity); | ||
| cityResetBtn.addEventListener('click', resetCityName); |
There was a problem hiding this comment.
Consider calling the event handler once on page load so that the city name starts the same as when reset.
| //'reset city name' button click | ||
| const resetCityName = () => { | ||
| const cityNameInput = document.getElementById('city-input'); | ||
| cityNameInput.value = 'Seattle'; //input field default value |
There was a problem hiding this comment.
Prefer using a constant that holds the default city name rather than hardcoding the literal string here.
3 participants
No description provided.