C22- Elzara and Priyanka- weather-report #2

PriyankaKaramchandani · 2024-12-04T18:20:54Z

No description provided.

…dOneTemp and reduceOneTemp with click.

yangashley

Nice work on weather-report, team!

yangashley · 2024-12-13T17:11:14Z

src/index.js

+const tempDisplay = document.getElementById('tempValue');
+const increaseTemp = document.getElementById('increaseTempControl');
+const decreaseTemp = document.getElementById('decreaseTempControl');
+const landscape = document.getElementById('landscape');
+const cityNameDisplay = document.getElementById('headerCityName');
+const cityNameInput = document.getElementById('cityNameInput')
+const tempButton = document.getElementById('currentTempButton');
+const skySelect = document.getElementById('skySelect');
+const gardenContent = document.getElementById('gardenContent');
+const skyDisplay = document.getElementById('sky');
+const resetButton = document.getElementById('cityNameReset');


I like that you organized all these variables related to page elements in one place. It makes your file more organized which ultimately makes your codebase more maintainable.

For variable names, it's more descriptive and would make your code more self-documenting to include the name of the element type where possible (which you do on line 11).

Suggestions for lines 2-3.

const increaseTempButton = document.getElementById('increaseTempControl'); const decreaseTempButton = document.getElementById('decreaseTempControl');

yangashley · 2024-12-13T17:14:24Z

src/index.js

+const updateTempColor = () => {
+tempDisplay.classList.remove('red', 'orange', 'yellow', 'green', 'teal');
+
+if (temperature >= 80) {
+    tempDisplay.classList.add('red');
+} else if (temperature >= 70) {
+    tempDisplay.classList.add('orange');
+} else if (temperature >= 60) {
+    tempDisplay.classList.add('yellow');
+} else if (temperature >= 50) {
+    tempDisplay.classList.add('green');
+} else {
+    tempDisplay.classList.add('teal');
+}
+};


Be mindful of indentation

Suggested change

const updateTempColor = () => {

tempDisplay.classList.remove('red', 'orange', 'yellow', 'green', 'teal');

if (temperature >= 80) {

tempDisplay.classList.add('red');

} else if (temperature >= 70) {

tempDisplay.classList.add('orange');

} else if (temperature >= 60) {

tempDisplay.classList.add('yellow');

} else if (temperature >= 50) {

tempDisplay.classList.add('green');

} else {

tempDisplay.classList.add('teal');

}

};

const updateTempColor = () => {

tempDisplay.classList.remove('red', 'orange', 'yellow', 'green', 'teal');

if (temperature >= 80) {

tempDisplay.classList.add('red');

} else if (temperature >= 70) {

tempDisplay.classList.add('orange');

} else if (temperature >= 60) {

tempDisplay.classList.add('yellow');

} else if (temperature >= 50) {

tempDisplay.classList.add('green');

} else {

tempDisplay.classList.add('teal');

}

};

Instead of using if/else if/else which can be brittle and bug-prone, consider using a list to store the landscape emojis. Then you can write logic that compares the first element in each inner list against temperature and grab the color or emojis.

temperatureColorsAndLandscapes = [ [80, "red", "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂"], [70, "orange", "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷"], [60, "yellow", "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃"], [50, "green", "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"], [40, "teal", "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲"], ]

yangashley · 2024-12-13T18:04:47Z

src/index.js

+    if (temperature >= 80) {
+        landscapeDisplay = '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂';
+    } else if (temperature >= 70) {
+        landscapeDisplay = '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷';
+    } else if (temperature >= 60) {
+        landscapeDisplay = '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'; 
+    } else {
+        landscapeDisplay = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲';
+    }


See comment above about incorporating a data structure to iterate over to get values instead of needing conditional logic.

yangashley · 2024-12-13T18:08:49Z

src/index.js

+const addOneTemp = () => {
+    temperature += 1;
+    tempDisplay.innerText = temperature;
+
+    updateDisplay()
+}; 
+
+const reduceOneTemp = () => {
+    temperature -= 1;
+    tempDisplay.innerText = temperature;
+
+    updateDisplay()
+};


Notice that the difference between these functions is += and -=.

Consider combining this logic into one helper function handleTempChange that could be registered with both the ⬆️ and ⬇️ buttons.

const handleTempChange = (event) => { if (event.target.id === 'increaseTempControl') { temperature ++; } else { temperature--; } updateDisplay(); };

yangashley · 2024-12-13T18:12:21Z

src/index.js

+increaseTemp.addEventListener('click', addOneTemp);
+decreaseTemp.addEventListener('click', reduceOneTemp);
+
+const updateCityName = () => {
+    const cityText = cityNameInput.value;
+    cityNameDisplay.innerText = cityText;
+};
+
+cityNameInput.addEventListener('input', updateCityName);


I like that you put all this callback logic into helper functions and then you register them with an event by function name (instead of just passing in a long anonymous function which can be hard to read and debug).

One thing that would help make this file more organized is to have all your addEventListener function calls in one place. As the code is written right now, these calls are sprinkled throughout and one has to look for them instead of just going to one place and know they're all there.

I'd recommend creating a helper called like registerEventHandlers and it could be called on page load

const registerEventHandlers () => { increaseTemp.addEventListener('click', addOneTemp); decreaseTemp.addEventListener('click', reduceOneTemp); cityNameInput.addEventListener('input', updateCityName); // etc etc }; document.addEventListener('DOMContentLoaded', () => { registerEventHandlers(); };

yangashley · 2024-12-13T18:19:14Z

src/index.js

+skyOptions.forEach(option => {
+    const optionElement = document.createElement('option');
+    optionElement.value = option.name.toLowerCase();
+    optionElement.textContent = option.name;
+    skySelect.appendChild(optionElement);
+});


Prefer this to be in a function and call the function where needed

yangashley · 2024-12-13T18:20:33Z

src/index.js

+tempButton.addEventListener('click', getCurrentTemp);
+
+// sky options with corresponding clouds
+const skyOptions = [


Constant variables should be named with capital letters to convey that the values should not be changed.

Suggested change

const skyOptions = [

const SKY_OPTIONS = [

yangashley · 2024-12-13T18:22:10Z

src/index.js

+        const temperatureKelvin = weatherData.main.temp;
+        temperature = Math.round((temperatureKelvin - 273.15) * 9/5 + 32); 


Prefer this logic to be in a helper function that gets called here. Invoking a descriptively named helper function would make the logic here more readable and also be concerned with doing one less thing.

const convertTemp = (temperatureKelvin) => { return Math.round((temperatureKelvin - 273.15) * 9/5 + 32); };

yangashley · 2024-12-13T18:23:35Z

src/index.js

+    return axios
+        .get(`http://localhost:5000/weather?lat=${lat}&lon=${lon}`)


Prefer url string be referenced by a constant variable

Suggested change

return axios

.get(`http://localhost:5000/weather?lat=${lat}&lon=${lon}`)

const baseURL = 'http://localhost:5000';

return axios

.get(`${baseURL}/weather?lat=${lat}&lon=${lon}`)

yangashley · 2024-12-13T18:23:56Z

src/index.js

+// function that gets location 
+const getLocation = (cityName) => {
+    return axios
+        .get(`http://localhost:5000/location?q=${cityName}`)


See comment above about constant var for base url

PriyankaKaramchandani and others added 11 commits December 2, 2024 14:45

Add index.html, css.html files. Implement wave 2 functionality for ad…

88418d6

…dOneTemp and reduceOneTemp with click.

refactor addOneTemp

bbf0019

finished wave 2, added hover pointer in css

fad312e

Wave 2 implementation

2133d3e

Merge branch 'main' of https://github.com/ielzara/weather-report

f62c0c9

Add wave 3 implementation.

b0cc19e

Wave 4 incomplete implementation.In progress.

fb33686

Debugged wave 4 implementation

47e9cb9

completed wave 5 implementation

f6d6da2

added a comment for wave 5

4e6dae2

Wave 6 implementation

0aa0cfb

yangashley reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C22- Elzara and Priyanka- weather-report #2

C22- Elzara and Priyanka- weather-report #2

PriyankaKaramchandani commented Dec 4, 2024

yangashley left a comment

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

yangashley Dec 13, 2024

		const temperatureKelvin = weatherData.main.temp;
		temperature = Math.round((temperatureKelvin - 273.15) * 9/5 + 32);

		return axios
		.get(`http://localhost:5000/weather?lat=${lat}&lon=${lon}`)

C22- Elzara and Priyanka- weather-report #2

Are you sure you want to change the base?

C22- Elzara and Priyanka- weather-report #2

Conversation

PriyankaKaramchandani commented Dec 4, 2024

yangashley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment