Sharks - Sana Pournaghshband#69
Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Great job! Your code was easy to read and I was able to do all of the required tasks in your weather app!
Please let me know if you have any questions about my comments
🟢 for weather-report!
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <link href="styles/index.css" rel="stylesheet"> | ||
| <!-- <script src="./src/index.js"></script> --> | ||
| <script src="./node_modules/axios/dist/axios.min.js"></script> |
There was a problem hiding this comment.
Looks like you have two script tags that brings loads axios. This same element is also on line 78.
Be sure to remove this script at the beginning of the page since "When the browser encounters a <script> tag, it stops loading the HTML document. Instead, the browser pauses to download the entire script, which might take a long time to load. The browser continues rendering the page only after the script has finished downloading." (from our Learn reading)
| <section id="location"> | ||
| <button id="reset" class="clicky"> | ||
| Reset | ||
| </button> | ||
|
|
||
| <form id="search"> | ||
| <input type="text" id="city" class="box" value="London"> | ||
| <button id="go" class="clicky" type="submit"> | ||
| Go | ||
| </button> | ||
| </form> | ||
|
|
||
|
|
||
| <select id="skySelect" class="box"> | ||
| <option>Lover</option> | ||
| <option>rep</option> | ||
| <option>1989</option> | ||
| <option>Red</option> | ||
| <option>Speak Now</option> | ||
| <option>fearless</option> | ||
|
|
||
| </select> | ||
| </section> |
There was a problem hiding this comment.
I think that these elements could be nested with the main element since this is content is a main feature of your weather report page.
| const convertKtoC = (temp) => temp - 273.15; | ||
| const convertKtoF = (temp) => (temp - 273.15) * (9 / 5) + 32; | ||
| const convertCtoF = (temp) => temp * (9 / 5) + 32; | ||
| const convertFtoC = (temp) => (temp - 32) * (5 / 9); |
| }, | ||
| }) | ||
| .then((response) => { | ||
| console.log(response.data); |
There was a problem hiding this comment.
Remember to remove print statements that you used for debugging, it helps keep your code concise and readable.
|
|
||
| const getLatAndLon = (city) => { | ||
| return axios | ||
| .get('http://127.0.0.1:5000/location', { |
There was a problem hiding this comment.
Consider using a constant variable like LOCATION_IQ_URL here instead of a string literal for the URL.
| const changeSky = (event) => { | ||
| const inputSky = document.getElementById('skySelect'); | ||
|
|
||
| const bg = document.getElementsByTagName('body')[0]; |
There was a problem hiding this comment.
We write code for other humans to read, even though bg is shorthand for background I would still name this variable background or skyColor to make it clear what this variable is.
| case 'Lover': | ||
| bg.style.background = 'url("assets/lover.jpeg")'; | ||
| break; | ||
| case 'rep': | ||
| bg.style.background = 'url("assets/rep backup.jpeg")'; | ||
| break; | ||
| case '1989': | ||
| bg.style.background = 'url("assets/1989 sky.jpeg")'; | ||
| break; | ||
| case 'Red': | ||
| bg.style.background = 'url("assets/red.jpeg")'; | ||
| break; | ||
| case 'Speak Now': | ||
| bg.style.background = 'url("assets/speak now.jpeg")'; | ||
| break; | ||
| case 'fearless': | ||
| bg.style.background = 'url("assets/fearless.jpeg")'; | ||
| break; |
There was a problem hiding this comment.
Check out the logic on lines 94-111, see how similar the branches are of the switch statement. We look for some input value and then pick a background accordingly. What if we had a list of objects that we could iterate through to find the values. We could set up something like
const SKY_BACKGROUND = [
{ input: 'Speak Now', background: 'url("assets/speak now.jpeg")'},
{ input: 'fearless', background: 'url("assets/fearless.jpeg")}
];Then your method would iterate through SKY_BACKGROUND and find the first record that matches the input value, then use it as the source to pick which background to render, which would help make the function a bit more concise.
This could accommodate a scenario where you might have even more backgrounds in the future, which would prevent you from having a really long switch statement
| bg.style.backgroundSize = 'cover'; | ||
| }; | ||
|
|
||
| const colorChange = () => { |
There was a problem hiding this comment.
Consider renaming this method to be more descriptive like setColorAndOutfit
| if (temp >= 80) { | ||
| tempColor = 'red'; | ||
| currentOutfit = 'assets/80.jpeg'; | ||
| lyric = 'Shake it Off'; | ||
| } else if (temp >= 70) { | ||
| tempColor = 'orange'; | ||
| currentOutfit = 'assets/75.jpeg'; | ||
| lyric = 'It feels like a perfect night to dress up like hipsters'; | ||
| } else if (temp >= 60) { | ||
| tempColor = 'yellow'; | ||
| currentOutfit = 'assets/50.jpeg'; | ||
| lyric = 'Autumn leaves falling down like pieces into place'; | ||
| } else if (temp >= 50) { | ||
| tempColor = 'green'; | ||
| currentOutfit = 'assets/60.jpg'; | ||
| lyric = 'There is something about the rain'; | ||
| } else { | ||
| tempColor = 'teal'; | ||
| currentOutfit = 'assets/49.jpeg'; | ||
| lyric = 'Forever Winter if you go'; | ||
| } |
There was a problem hiding this comment.
Like I mentioned in your changeSky method, you can use a list here to contain objects that have the color, outfit and lyric values for each temperature range. Then your method would iterate through the constant list and find the first record that has an upper bound higher than our temperature, then use it as the source of picking the color/outfit/lyric.
| const switchCAndF = (event) => { | ||
| state.temperature = state.isF | ||
| ? convertFtoC(state.temperature) | ||
| : convertCtoF(state.temperature); | ||
| state.isF = !state.isF; | ||
|
|
||
| const unit = document.getElementById('unit'); | ||
| unit.textContent = state.isF ? '°F' : '°C'; | ||
|
|
||
| colorChange(); | ||
| }; |
2 participants
No description provided.