Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C22 - Phoenix - Luqi - Sphinx - Lorraine #8

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+351 −1
Open

C22 - Phoenix - Luqi - Sphinx - Lorraine #8

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
eedb12a
wave 01 completed
Dec 2, 2024
80c10c3
add increase/decrease button event handler
Dec 2, 2024
0c9f008
wave 02 completed
Dec 3, 2024
a3224b8
refactor changecolorand landscape function
Dec 3, 2024
19b1a68
wave 03 completed
Dec 3, 2024
64c61f9
wave4 in progress
shiqipaper Dec 3, 2024
568bdfd
wave4 debug
shiqipaper Dec 3, 2024
51c32ac
complete wave4
shiqipaper Dec 3, 2024
a99f030
wave 04 completed
Dec 3, 2024
3c35895
complete wave5
shiqipaper Dec 3, 2024
f49daa3
complete wave6
shiqipaper Dec 4, 2024
6c55ed7
fix some bugs in findlatitudeandlogitude and resetcityname
shiqipaper Dec 4, 2024
4ad3c2a
add citynameinputeventhandler
shiqipaper Dec 4, 2024
afcf81e
add weather garden background
Dec 5, 2024
e2a5853
change city name button event
Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix some bugs in findlatitudeandlogitude and resetcityname
@shiqipaper
shiqipaper committed Dec 4, 2024
commit 6c55ed72478f8a98be15853881170d45832a928b
27 changes: 15 additions & 12 deletions src/index.js
View file
Original file line number Diff line number Diff line change
@@ -67,11 +67,11 @@ const findLatitudeAndLongitude = () => {
.then((response) => {
latitude = response.data[0].lat;
longitude = response.data[0].lon;
const temp = findWeather(latitude, longitude);
changeColorAndLandscape(temp)
findWeather(latitude, longitude);
changeColorAndLandscape(state.curTemp)
Comment on lines +70 to +71
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than chaining this directly into the next API call (which should be returned so that we wait for the promise to complete), just return the latitude/longitude data. Focus on this helper function just being a wrapper around the API call, but which hides the details fo that from the rest of the application. This should be a function that (asynchronously) accepts a city name, and returns its lat/lon data. Nothing more.

We can focus the other API call to similarly accept lat/lon data and return temperature data.

We could then have a third function whose responsibility is to chain those behaviors together, creating a function that accepts a city name and returns a temperature.

Finally, a fourth function could be responsible for calling the city to temperature function, and using the temperature result to update the temperature state, and publish that result to the UI.

Keeping individual functions focused on a single task, and adding more functions whose main role is to combine the effects of multiple functions is a good approach for structuring complex code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that as written, the call to changeColorAndLandscape is not waiting for the result of findWeather to be available, which is why we're not seeing the color and landscape update when the new temperature eventually comes back. findWeather is making an axios call, which will happen asynchronously after this function completely finishes running, so there's no possibility that it could finish before the call to changeColorAndLandscape.

})
.catch((error) => {
console.log('error in findLatitudeAndLongitude!');
console.log('error in testfindLatitudeAndLongitude!');
});
}

@@ -85,23 +85,27 @@ const findWeather = (latitude, longitude) => {
state.curTemp = fahrenheitTemp;
const temp = document.getElementById("tempValue");
temp.textContent = state.curTemp;

})

.catch((error) => {
console.log('error in Weather!');
});
}

const resetCityName = () => {
const defaultCity = 'Seattle';
const cityName = document.getElementById("cityNameInput");
const headerCityName = document.getElementById("headerCityName")
cityNameInput.value = defaultCity;
headerCityName.textContent = defaultCity;
findLatitudeAndLongitude(defaultCity);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was no requirement to update the realtime data when resetting the city, but it's easy to do since we have a helper function to handle that. Still, it's a little surprising that it does happen here, when it doesn't happen as we type the city name (we wait until the Get Realtime Temperature button is clicked).

};

const changeCityName = () => {
const cityName = document.getElementById("cityNameInput").value;
const defaultCity = 'Seattle';
if (cityName === '') {
document.getElementById("headerCityName").textContent = defaultCity;
} else {
document.getElementById("headerCityName").textContent = cityName;
};

findLatitudeAndLongitude(defaultCity);
};

const registerEventHandlers = () => {
@@ -112,7 +116,7 @@ const registerEventHandlers = () => {
decreaseButton.addEventListener("click", decreaseTemp);

const cityResetButton = document.getElementById("cityNameReset");
cityResetButton.addEventListener("click", changeCityName);
cityResetButton.addEventListener("click", resetCityName);

const updateTempButton = document.getElementById('currentTempButton');
updateTempButton.addEventListener('click', findLatitudeAndLongitude);
@@ -123,5 +127,4 @@ const registerEventHandlers = () => {


document.addEventListener("DOMContentLoaded", registerEventHandlers);
document.addEventListener("DOMContentLoaded", changeCityName);
document.addEventListener("DOMContentLoaded", findLatitudeAndLongitude);
document.addEventListener("DOMContentLoaded", resetCityName);