Skip to content

Masi Nakhjiri-Cheetahs Group#75

Open
masinakh wants to merge 5 commits intoAda-C18:mainfrom
masinakh:main
Open

Masi Nakhjiri-Cheetahs Group#75
masinakh wants to merge 5 commits intoAda-C18:mainfrom
masinakh:main

Conversation

@masinakh
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@jbieniosek jbieniosek left a comment

Choose a reason for hiding this comment

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

Great work on this project Masi! The code you wrote for wave 4 is really close, I've put in a suggested solution, but I didn't have time to test it so it may need some adjustments. The code in this project is really clean and easy to read. This project is green.

Comment thread src/index.js
Comment on lines +58 to +65
if (e.target.id == 'tempUp') {
state.temp += 1;
console.log(state);
showTemperature();
} else if (e.target.id == 'tempDown') {
state.temp -= 1;
showTemperature();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great solution, nice use of the event target id!

Comment thread src/index.js
Comment on lines +134 to +135
increaseArrow.addEventListener('click', changeTemp);
increaseArrow.addEventListener('click', tempColorAndLandscape);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works, but I recommend refactoring so that changeTemp calls tempColorAndLandscape rather than adding tempColorAndLandscape as a separate event listener. tempColorAndLandscape relies on the change to state.temp that happens in changeTemp, but adding it as a separate event listener implies that these are two completely separate actions, rather than connected.

Comment thread src/index.js
state.temp -= 1;
showTemperature();
}
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please see my note on line 135, this is the refactor change that I recommend.

Suggested change
};
tempColorAndLandscape();
};

Comment thread src/index.js
Comment on lines +84 to +122
// const getWeather = async () => {
// let latitude, longitude;

// await axios
// .get('http://127.0.0.1:5000/location', {
// params: {
// q: state.city,
// },
// })
// .then((response) => {
// latitude = response.data[0].lat;
// longitude = response.data[0].lon;
// })
// .catch((error) => {
// console.log('error in finding location!');
// });

// axios
// .get('http://127.0.0.1:5000/weather', {
// params: {
// lat: latitude,
// lon: longitude,
// },
// })
// .then((response) => {
// // console.log(response.data);
// const kelvinTemperature = response.data.main.temp;
// // console.log(kelvinTemperature);
// const fahrenheitTemperature = Math.round(
// (kelvinTemperature - 273.15) * 1.8 + 32
// );
// state.temp = fahrenheitTemperature;
// tempValue.innerText = `${state.temp + '\u00B0F'}`;
// tempColorAndLandscape();
// })
// .catch((error) => {
// console.log('error in finding temperature!', error);
// });
// };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is so close to working! I think with a few changes (although I didn't test it) you could get it to work. The big change is moving the second axios call to a separate function, and then calling that function inside the .then handler of the first axios call:

Suggested change
// const getWeather = async () => {
// let latitude, longitude;
// await axios
// .get('http://127.0.0.1:5000/location', {
// params: {
// q: state.city,
// },
// })
// .then((response) => {
// latitude = response.data[0].lat;
// longitude = response.data[0].lon;
// })
// .catch((error) => {
// console.log('error in finding location!');
// });
// axios
// .get('http://127.0.0.1:5000/weather', {
// params: {
// lat: latitude,
// lon: longitude,
// },
// })
// .then((response) => {
// // console.log(response.data);
// const kelvinTemperature = response.data.main.temp;
// // console.log(kelvinTemperature);
// const fahrenheitTemperature = Math.round(
// (kelvinTemperature - 273.15) * 1.8 + 32
// );
// state.temp = fahrenheitTemperature;
// tempValue.innerText = `${state.temp + '\u00B0F'}`;
// tempColorAndLandscape();
// })
// .catch((error) => {
// console.log('error in finding temperature!', error);
// });
// };
const getWeather = () => {
//let latitude, longitude;
axios
.get('http://127.0.0.1:5000/location', {
params: {
q: state.city,
},
})
.then((response) => {
const latitude = response.data[0].lat;
const longitude = response.data[0].lon;
getTempForLocation(latitude, longitude);
})
.catch((error) => {
console.log('error in finding location!');
});
};
const getTempForLocation = (latitude, longitude) => {
axios
.get('http://127.0.0.1:5000/weather', {
params: {
lat: latitude,
lon: longitude,
},
})
.then((response) => {
// console.log(response.data);
const kelvinTemperature = response.data.main.temp;
// console.log(kelvinTemperature);
const fahrenheitTemperature = Math.round(
(kelvinTemperature - 273.15) * 1.8 + 32
);
state.temp = fahrenheitTemperature;
tempValue.innerText = `${state.temp + '\u00B0F'}`;
tempColorAndLandscape();
})
.catch((error) => {
console.log('error in finding temperature!', error);
});
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants