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

Show file tree
Hide file tree
Changes from all commits
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
45 changes: 44 additions & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,58 @@
<!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"></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="------">------</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>
<div id="landscape"></div>
</div>
</section>
<script src="https://unpkg.com/axios/dist/axios.min.js"></script>
<script src="./src/index.js"></script>
</body>
</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.

134 changes: 134 additions & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
"use strict";

const state = {
curTemp: 60,
}

const increaseTemp = () => {
const temp = document.getElementById("tempValue");
Copy link

Choose a reason for hiding this comment

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

Rather than re-getting the elements I need each time a callback gets run, I like to fetch everything once one the page loads and store the references in some globals or in the state structure. That simplifies the code in the venet callbacks a bit, and removes the chance of a typo in the selector leading to problems.

state.curTemp += 1;
temp.textContent = state.curTemp;
changeColorAndLandscape(state.curTemp);
Comment on lines +8 to +11
Copy link

Choose a reason for hiding this comment

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

Consider restructuring this function (and the decrease) so that all this does is modify the value of state.curTemp and then call another function to update any UI that depends on the current temperature value. Here, that would mostly mean combining the textContent logic, and the call to changeColorAndLandscape into a single helper (maybe publishTemperatureValue). This could be called in the increase/decrease, as well as after the live weather lookup rather than needing to remember to do multiple things in all those places.

};

const decreaseTemp = () => {
const temp = document.getElementById("tempValue");
state.curTemp -= 1;
temp.textContent = state.curTemp;
changeColorAndLandscape(state.curTemp);
};

const changeColorAndLandscape = (temp) => {
Copy link

Choose a reason for hiding this comment

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

Rather than having a function that directly modifies color and landscape elements, consider restructuring this into two functions, one that determines the color and lanscape to use based on the temperature, and another that actuall applies those values to the UI. Note that currently, each branch has a lot of repetition in the stpes it's doing, with the only difference being the values used

const tempValue = document.getElementById("tempValue");
const landscape = document.getElementById("landscape");
if (temp >= 80) {
tempValue.style.color = "Red";
Copy link

Choose a reason for hiding this comment

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

Setting the style attribute is the equivalent of hard coding a style in HTML. In general, prefer to assign a class to an element, and use style rules to effect any desired UI formatting changes.

landscape.textContent = "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂";
} else if (temp >= 70) {
tempValue.style.color = "Orange";
landscape.textContent = "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷";
} else if (temp >= 60) {
tempValue.style.color = "Yellow";
landscape.textContent = "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃";
} else if (temp >= 50) {
tempValue.style.color = "Green";
landscape.textContent = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲";
} else {
tempValue.style.color = "Teal";
landscape.textContent = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲";
}
Comment on lines +24 to +39
Copy link

Choose a reason for hiding this comment

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

This large chained if statement is repetative. If we stored the relevant data in a way that we could loop through (direct key lookup isn't as helpful, since our temperatures have ranges) we could make our application more data driven, so that just by changing a data structure (that maybe we could load from an API call) there could be different temperature behaviors.

Consider how we could use a structure like the following:

// each row holds the bottom of the temperature range, a class name, and a landscape string
const TEMPERATURE_DATA = [
    [80, 'very-hot-temp', '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'],
    [70, 'hot-temp', '🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷'],
    [60, 'warm-temp', '🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃'],
    [50, 'cool-temp', '🌲🌲🍂🌲🍁🍃🌲🌾🌲🌲🍂🍁🌲'],
    [40, 'cold-temp', '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲'],
];

};

const updateSky = () => {
const skySelect = document.getElementById("skySelect").value;
const sky = document.getElementById("sky")
if (skySelect === "Sunny") {
sky.textContent = "☁️ ☁️ ☁️ ☀️ ☁️ ☁️";
Comment on lines +45 to +46
Copy link

Choose a reason for hiding this comment

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

Here, we're mapping from a string value to a string to be displayed. We could store this as key-value pairs and look up the sky value to use using a data structure rather than having a long if/else chain.

} else if (skySelect === "cloudy") {
sky.textContent = "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️";
} else if (skySelect === "rainy") {
sky.textContent = "🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧";
} else if (skySelect === "snowy") {
sky.textContent = "🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨";
} else {
sky.textContent = "----------------------------- ";
}
};

const findLatitudeAndLongitude = () => {
let latitude, longitude;
const cityName = document.getElementById("headerCityName").textContent;
axios.get('http://127.0.0.1:5000/location',
Copy link

Choose a reason for hiding this comment

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

Rather than hard-coding the URL, it can be helpful to store the base url in a variable that we also interpolate into the string here. This can make deployment easier (since we'll need to change any dev-routed endpoints to point to their production locations).

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

then here

    axios.get(`${BASE_URL}/location`, {

Copy link

Choose a reason for hiding this comment

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

Be sure to return the promise chain. The only way to setup any code that needs to wait for this to finishbefore it runs is to get access to the promise chain we build here, and then .then chain onto the end of it. Never leave a promise chain "trapped" inside a helper method. Always return it.

{
params: {
q: cityName,
}
})
Comment on lines +62 to +66
Copy link

Choose a reason for hiding this comment

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

👍 Nice use of the params option value. This prevents a user from potentially injecting their own query params into the request (if we were to use string interpolation to add the query params). Here it wouldn't do much, but there could be situations where users could cause malicious behaviors. In general, if a library allows us to set params/arguments through a set of function arguments, we should use that over building our own string. Building an "executable" string directly from user input is a common way that hackers compromise systems.

.then((response) => {
latitude = response.data[0].lat;
longitude = response.data[0].lon;
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!');
});
}

const findWeather = (latitude, longitude) => {
axios.get('http://127.0.0.1:5000/weather', {
Copy link

Choose a reason for hiding this comment

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

Here again we would want to return the promise chain. This would allow us to chain additional logic after the API call completes outside this function, rather than having this function need to be responsible both for making the API call and updating the UI. This function should only make the API call, and code elsewhere should chain onto the end of this promise chain with .then to add the logic that updates the UI.

params: { lat: latitude, lon: longitude },
})
.then((response) => {
const kelvinTemp = response.data.main.temp;
const fahrenheitTemp = Math.round(kelvinTemp * 9 / 5 - 459.67);
Copy link

Choose a reason for hiding this comment

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

Consider moving the kelvin to fahrenheit conversion logic to a helper function.

state.curTemp = fahrenheitTemp;
const temp = document.getElementById("tempValue");
const weather = document.getElementById("skySelect");
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")
cityName.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 headerCityName = document.getElementById("headerCityName");
Comment on lines +106 to +107
Copy link

Choose a reason for hiding this comment

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

Rather than re-getting the elements I need each time a callback gets run, I like to fetch everything once one the page loads and store the references in some globals or in the state structure. That simplifies the code in the event callbacks a bit, and removes the chance of a typo in the selector leading to problems.

headerCityName.textContent = cityName;
Copy link

Choose a reason for hiding this comment

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

Simliar to my suggestion for updating values based on the temperature with a helper function, we could make a helper for updating the city name in the UI as well. Even though it's only a single step, it would still be more self-documenting to have a few function calls to something like publishCityName rather than scattered calls to set this textContent. And if we made a change in what happened when publishing the city name, we would then only need to update it in that one function rather than find every place where we updated the textContent directly.

// findLatitudeAndLongitude(cityName);
};

const registerEventHandlers = () => {
const increaseButton = document.getElementById("increaseTempControl");
increaseButton.addEventListener("click", increaseTemp);

const decreaseButton = document.getElementById("decreaseTempControl");
decreaseButton.addEventListener("click", decreaseTemp);

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

const cityNameInput = document.getElementById("cityNameInput");
cityNameInput.addEventListener("input", changeCityName);

const updateTempButton = document.getElementById('currentTempButton');
updateTempButton.addEventListener('click', findLatitudeAndLongitude);

const skySelect = document.getElementById("skySelect");
skySelect.addEventListener("change", updateSky);
};


document.addEventListener("DOMContentLoaded", registerEventHandlers);
document.addEventListener("DOMContentLoaded", resetCityName);
Comment on lines +133 to +134
Copy link

Choose a reason for hiding this comment

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

We can register multiple things to happen in response to an event, but typically, we'll just make a single function that calls the several things we'd like to do. So here, we could make a function setupPage that itself calls registerEventHandlers and resetCityName.

167 changes: 167 additions & 0 deletions styles/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
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;
height: 200px;
width: 500px;
background-color: lightblue;
}

.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;
}