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 Dana Delgado & Rong Jiang #11

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

C22 Dana Delgado & Rong Jiang #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e8fc589
"wave-01"
dmdelg Dec 3, 2024
a718a2c
wave 2 update temp value
Dec 3, 2024
47247e3
wave 03 and 06
dmdelg Dec 3, 2024
f8b6455
Merge branch 'main' of https://github.com/dmdelg/weather-report
dmdelg Dec 3, 2024
d7448c8
add wave 2 landscapes
Dec 3, 2024
9ead02d
wave 5 and formatting
Dec 4, 2024
ea1fee5
implementation of wave 4
dmdelg Dec 5, 2024
b8a4193
implementation of wave 04
dmdelg Dec 5, 2024
bb0aa27
Merge branch 'main' of https://github.com/dmdelg/weather-report
dmdelg Dec 5, 2024
7fe7086
final changes
dmdelg 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
49 changes: 47 additions & 2 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,60 @@
<!DOCTYPE html>
<html lang="en">

<head>
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Weather Report</title>
<link rel="preconnect" href="https://fonts.gstatic.com">
<link href="https://fonts.googleapis.com/css2?family=Rubik&display=swap" rel="stylesheet">
<link rel="stylesheet" href="styles/index.css" />
<link rel="stylesheet" href="styles/index.css">
</head>

<body>
<header class="header__header">
<h1>Weather Report</h1>
<span>For the lovely city of
<span id="headerCityName" class="header__city-name"></span></span>
</header>
<section class="temperature__section">
<h2>Temperature</h2>
<div class="temperature__content">
<div class="temperature__controls">
<span id="increaseTempControl">⬆️</span>
<span id="tempValue">70°F</span>
<span id="decreaseTempControl">⬇️</span>
</div>
<button id="currentTempButton">Get Realtime Temperature</button>
</div>
</section>
<section class="sky__section">
<h2>Sky</h2>
<select id="skySelect">
<option value="">--Please choose a sky--</option>
<option value="sunny">Sunny</option>
<option value="cloudy">Cloudy</option>
<option value="rainy">Rainy</option>
<option value="snowy">Snowy</option>
</select>
</section>

<section class="city-name__section">
<h2>City Name</h2>
<input type="text" id="cityNameInput">
<button id="cityNameReset" class="city-name__reset-btn">Reset</button>
</section>

<section class="garden__section">
<h2>Weather Garden</h2>
<div id="gardenContent" class="garden__content">
<div id="sky"></div>
<span id="skyDisplay"></span>
<div id="landscape"></div>
<span id="landscapeDisplay"></span>
</div>
</section>
<script src="https://unpkg.com/axios/dist/axios.min.js"></script>
<script src="./src/index.js"></script>
</body>
</html>
</html>
6 changes: 6 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

137 changes: 137 additions & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
// wave 2 change temperature
const tempValue = document.getElementById('tempValue');
const tempIncrease = document.getElementById('increaseTempControl');
const tempDecrease = document.getElementById('decreaseTempControl');
const landscape = document.getElementById('landscapeDisplay');
Comment on lines +2 to +5
Copy link

Choose a reason for hiding this comment

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

It's descriptive and makes your code self-documenting when your variable names also indicate what the element is.

Suggested change
const tempValue = document.getElementById('tempValue');
const tempIncrease = document.getElementById('increaseTempControl');
const tempDecrease = document.getElementById('decreaseTempControl');
const landscape = document.getElementById('landscapeDisplay');
const tempValueDisplay = document.getElementById('tempValue');
const tempIncreaseButton = document.getElementById('increaseTempControl');
const tempDecreaseButton = document.getElementById('decreaseTempControl');
const landscapeDisplay= document.getElementById('landscapeDisplay');


let currentTemp = 70;
let currentLandscape = '';

function updateTemperature() {
Copy link

Choose a reason for hiding this comment

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

Recall that we can also use minimized syntax that comes from ES6 with arrow functions syntax. For our curriculum, we prefer the arrow syntax.

const updateTemperature = () => {
  // your logic here
}

tempValue.textContent = `${currentTemp}°F`;

tempValue.classList.remove("temp-red", "temp-orange", "temp-yellow", "temp-green", "temp-teal");
Copy link

Choose a reason for hiding this comment

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

Why do we need this line? I ask because on page load this element does not have any of these classes associated with it and I also don't see a place where these classes get added (and would later need to be removed).

If this line isn't needed for your logic then it should be removed.


if (currentTemp >= 80) {
tempValue.classList.add("red");
currentLandscape = "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂";
} else if (currentTemp >= 70) {
tempValue.classList.add("orange");
currentLandscape = "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷";
} else if (currentTemp >= 60) {
tempValue.classList.add('yellow');
currentLandscape = '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃';
} else if (currentTemp >= 50) {
tempValue.classList.add("green");
currentLandscape = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲";
} else if (currentTemp <= 49) {
tempValue.classList.add("teal");
currentLandscape = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲";
}
Comment on lines +15 to +30
Copy link

Choose a reason for hiding this comment

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

Like you use a data structure for skies, consider using a data structure here to store this info because a bunch of conditional logic like if/else if/else if/else, etc can be brittle and bug-prone.

A list could work here, then you can iterate over the list and check tempVaue against the first element of the list and get the color and landscape values.

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


landscape.textContent = currentLandscape;
}

tempIncrease.addEventListener('click', () => {
currentTemp += 1;
updateTemperature();
});

tempDecrease.addEventListener("click", () => {
currentTemp -= 1;
updateTemperature();
});
Comment on lines +35 to +43
Copy link

Choose a reason for hiding this comment

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

Notice that the difference between these two sections of code is += and -=. Consider creating a helper function that could handle increasing and decreasing the temperature and registering the callback for both buttons.

const handleTempChange = (event) => {
    if (event.target.id === 'increaseTempControl') {
       currentTemp += 1;
    } else {
       currentTemp -= 1;
    }
  updateTemperature();
};

Copy link

Choose a reason for hiding this comment

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

Prefer the logic that gets registered with an event to be written in a helper function and then you pass the function when you add an event listener.

It makes your code more readable and it can be easier to debug.


// Initialize the temperature and landscape display
updateTemperature();

// Wave 5
// change sky display when select from dropdown list
const skySelect = document.getElementById('skySelect');
const skyDisplay = document.getElementById('skyDisplay');
Comment on lines +50 to +51
Copy link

Choose a reason for hiding this comment

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

Consider organizing your file so that all the elements are in one section. I'd move lines 50-51 up to the top with lines 2-5.


const skyOptions = {
sunny: '☁️ ☁️ ☁️ ☀️ ☁️ ☁️',
cloudy: '☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️',
rainy: '🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧',
snowy: '🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨',
};

function updateSky(sky) {
const selectedSky = skySelect.value;
skyDisplay.textContent = skyOptions[selectedSky];
};

skySelect.addEventListener('change', updateSky);
Comment on lines +60 to +65
Copy link

Choose a reason for hiding this comment

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

I like how the logic on lines 61-62 are in a helper function and then you pass the function to addEventListener (instead of passing all of the logic in as an anonymous function on line 65).


updateSky();

// wave 3 & 6
document.addEventListener("DOMContentLoaded", () => {
Copy link

Choose a reason for hiding this comment

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

Did you mean for lines 77 - 136 to be written inside the anonymous function that gets passed to the addEventListener that registers this logic and executes on page load? If so, could you explain to me the logic about writing the code like this?

It seems unintentional because not all of the code is nested in the anonymous function here, but I want to check with you two.

If it was an accident, take care in the future to make sure your code is nested or not nested as you intend. If it is intentional, I'm wondering why the code outside of line 70 is not also inside this long anonymous function.

@dmdelg @cornetto9 ^^

const cityInput = document.getElementById("cityNameInput");
const cityDisplay = document.getElementById("headerCityName");
const resetButton = document.getElementById("cityNameReset");
const currentTempButton = document.getElementById("currentTempButton");

// Helper function to fetch location data
function getLocationData(cityName) {
return axios
.get(`http://localhost:5000/location`, {
Copy link

Choose a reason for hiding this comment

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

Prefer to refer to this url string with a constant variable. When possible refer to constant values like this with a variable name to make your code more self documenting and help other devs understand what's going on with your code.

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

return axios
    .get(`${baseURL}/location`)

params: { q: cityName },
})
.then((locationResponse) => {
if (!Array.isArray(locationResponse.data) || locationResponse.data.length === 0) {
throw new Error("No valid location data found.");
}
Comment on lines +83 to +85
Copy link

Choose a reason for hiding this comment

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

Prefer to use a catch block for error handling instead of manually throwing an error.

return locationResponse.data[0]; // Return first valid location
});
}

// Function to convert Kelvin to Fahrenheit
function kelvinToFahrenheit(kelvin) {
return Math.round((kelvin - 273.15) * 9 / 5 + 32);
}

// Event listener for the 'currentTempButton'
currentTempButton.addEventListener("click", () => {
const cityName = cityDisplay.textContent.trim();
if (!cityName) {
alert("Please set a city name before fetching the temperature.");
return;
}

// Fetch location data and weather data
getLocationData(cityName)
.then((locationData) => {
const { lat, lon } = locationData;
return axios.get(`http://localhost:5000/weather`, {
Copy link

Choose a reason for hiding this comment

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

Reference this string with a constant variable

params: { lat, lon },
});
})
.then((weatherResponse) => {
const temperature = weatherResponse.data.main.temp;
currentTemp = kelvinToFahrenheit(temperature); // Convert from Kelvin to Fahrenheit
updateTemperature(); // Update UI with the new temperature
})
.catch((error) => {
alert("Could not fetch weather data. Please try again.");
});
Comment on lines +116 to +118
Copy link

Choose a reason for hiding this comment

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

Nice job using a catch block here.

});

// Event listener for the city input
cityInput.addEventListener("keydown", (event) => {
if (event.key === "Enter") {
Comment on lines +122 to +123
Copy link

Choose a reason for hiding this comment

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

I would prefer this event to not be on keydown because you don't explicitly tell the user that they need to hit enter after typing the city. After I typed my city name, I didn't see it reflected in the UI and didn't see a button to press so I guessed that I should hit enter and finally saw the city name.

I'd suggest using input as the event.

cityInput.addEventListener("input", yourEventHandler);

const cityName = cityInput.value;
cityDisplay.textContent = cityName;
}
});
Comment on lines +122 to +127
Copy link

Choose a reason for hiding this comment

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

Currently you attach event handlers to elements throughout this file. Consider creating a single function that registers all the event listeners in one place, then that method could be invoked on page load. This would make it easier to find and read all the event handlers and elements that are associated with each other which would make your codebase more maintainable.

const registerEventHandlers = () => {
  cityInput.addEventListener("keydown", updateCityInput);
  resetButton.addEventListener("click", resetCity);
  // all other event handlers would be registered in here too.
  
};
document.addEventListener("DOMContentLoaded", registerEventHandlers);


// Event listener for the reset button
resetButton.addEventListener("click", () => {
cityInput.value = "";
cityDisplay.textContent = "";
Comment on lines +131 to +132
Copy link

Choose a reason for hiding this comment

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

Like my previous comments, prefer this logic to be in a descriptively named helper function which will make your code more organized, self-documenting, and easier to debug.

const resetCity = () => {
        cityInput.value = "";
        cityDisplay.textContent = "";
};

resetButton.addEventListener("click", resetCity);

});

// Initialize the temperature and landscape display
updateTemperature();
Copy link

Choose a reason for hiding this comment

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

It would be nice if you put all the functions that need to be invoked on page load into one function called like loadControls and then you just call loadControls once.

For example, updateSky() is called on line 67, updateTemperature() is called on line 46 and then it's called again here on line 136. Do we need it called twice or was it accidentally called twice?

Right now you have function calls sprinkled throughout index.js and it can be hard to follow what is happening and when it is happening. Having an organized file is important to maintain your codebase.

const loadControls = () => {
  updateSky()
  updateTemperature();
};

loadControls();

});
169 changes: 169 additions & 0 deletions styles/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
h2 {
margin: 0 auto 2rem auto;
}

body {
display: grid;
grid-template-columns: 1fr 2fr;
grid-template-rows: auto auto auto auto;
grid-gap: 1rem;

font-family: "Rubik", sans-serif;
font-size: 18px;
background-color: #1b69f9;
margin: 2rem;
}

.header__header {
color: white;
grid-column: span 3;
display: flex;
align-items: center;
margin: 2rem auto 3rem 0;
}

.header__header > h1 {
margin-right: 2rem;
font-size: 3em;
}

.header__city-name {
font-style: oblique;
font-size: 2rem;
}

.header__city-name::before,
.header__city-name::after {
content: "✨";
}

.temperature__section,
.sky__section,
.city-name__section {
border-radius: 8px;
padding: 2rem;
background-color: white;
}

.temperature__section {
grid-row: 2;
}

.temperature__section button {
background-color: #1b69f9;
border: none;
color: white;
padding: 15px 32px;
text-align: center;
text-decoration: none;
display: inline-block;
font-size: 16px;
border-radius: 10px
}

.sky__section {
grid-row: 3;
}

.city-name__section {
grid-row: 4;
}

.garden__section {
grid-row: 2 / span 3;
grid-column: 2;
text-align: center;
align-self: center;

}

.temperature__content {
display: flex;
flex-direction: row;
justify-content: space-around;
/* justify-content: center; */
}

#tempValue {
font-size: 3rem;
margin-left: 1.5rem;
/* padding-right: 1rem; */
/* margin-right: 1.5rem; */
}

.temperature__controls {
display: flex;
flex-direction: column;
align-items: center;
}

.garden__section > h2 {
color: white;
}

.garden__content {
min-height: 200px;
max-width: fit-content;
margin: auto;
padding: 2rem;

display: flex;
flex-direction: column;
justify-content: space-between;

border-radius: 8px;
font-size: 2em;

color: white;
background-color: rgba(252, 252, 252, 0.5);

}

.city-name__reset-btn {
border: 0;
background-color: #1655cc;
color: white;
border-radius: 8px;
padding: 1rem;
font-family: "Rubik", sans-serif;
}

.red {
color: red;
}

.orange {
color: orange;
}

.yellow {
color: gold;
}

.yellow-green {
color: yellowgreen;
}

.green {
color: green;
}

.teal {
color: teal;
}

.cloudy {
background-color: lightgrey;
}

.sunny {
background-color: rgb(221, 255, 255);
}

.rainy {
background-color: lightblue;
}

.snowy {
background-color: lightsteelblue;
}