getting rid of global state#1
Conversation
|
✔️ Deploy Preview for hardcore-fermat-e724bc ready! 🔨 Explore the source changes: d736ef0 🔍 Inspect the deploy log: https://app.netlify.com/sites/hardcore-fermat-e724bc/deploys/614b9303fa312e00075daa2f 😎 Browse the preview: https://deploy-preview-1--hardcore-fermat-e724bc.netlify.app |
Smelbows
left a comment
Smelbows
left a comment
There was a problem hiding this comment.
I put some comments so I could remember what I did!
| }; | ||
|
|
||
| //Creates a weather icon based on the forecasted weather | ||
| function chooseWeatherIcon(filteredForecast, i) { |
There was a problem hiding this comment.
this function didn't actually need two arguments, so we've just given it each day's forecast (from 'filteredForcast[i]) instead
| let main = forecast.weather[0].main; | ||
| if (main === "Clouds") { | ||
| weatherImg = cloudyIcon; | ||
| return cloudyIcon; |
There was a problem hiding this comment.
the function now returns an icon, which we assign to a variable up in the place we called it. (instead of making the assignment inside the function)
| const forecastWeather = filteredForecast[i].weather.description; | ||
| createFiveDaysInnerHTML(days, forecastTemp); | ||
| } | ||
| weatherContainer.innerHTML += createFiveDaysInnerHTML(days, forecastTemp, daysWeatherIcon); |
There was a problem hiding this comment.
the function now returns the HTML (instead of assigning it to weatherContainer.InnerHTML) the assignment now happens here.
| const timeOfSunrise = getTimeOf(data.sys.sunrise, timezone); | ||
| const timeOfSunset = getTimeOf(data.sys.sunset, timezone) | ||
| changeHeaderInnerHTML(data, timeOfSunrise, timeOfSunset); | ||
| const weatherIcon = chooseWeatherIcon(data) |
There was a problem hiding this comment.
The icon now comes from the chooseWeatherIcon function (same one that we used for the five day forecast). So now the background img function is only for background img
| function getBackgroundWeatherClass(data) { | ||
| let main = data.weather.map((item) => item.main); | ||
| if (main.includes("Clouds")) { | ||
| weatherImg = cloudyIcon; |
There was a problem hiding this comment.
All of these are taken out because the other if/else function was doing the same thing
| if (main.includes("Clouds")) { | ||
| weatherImg = cloudyIcon; | ||
| backgroundImg.classList = "clouds"; | ||
| return "clouds"; |
There was a problem hiding this comment.
these now return instead of assigning, the assigning is done when we call the function
|
|
||
|
|
||
| function createFiveDayForecast(filteredForecast) { | ||
| weatherContainer.innerHTML = "" |
There was a problem hiding this comment.
Clearing the HTML for the five days as the first thing before rewriting it (it was in the event listener before)
1 participant
Hey everyone! so I was asking Steve questions about refactoring (because apparently it's my favourite thing now) and I changed a few things on my local copy just to see what was possible. Then I thought, hey!!, we haven't done any branching or looking at pull requests yet, so why not! The changes don't affect how our site looks or runs in any way at all, so we can actually just ignore it if we want to, but if you'd like to we can look at the Javascript and pull it into the master. Essentially it takes away things that are in the global scope, and it uses just one function to get the weather icons for both the five day forecast and the main forecast. So the functions hopefully look simpler and easier to read without actually doing anything different. Btw, now I kind of want one of you to create another branch with some different changes just so that we have some merge conflicts to deal with (not really!!)