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

SL c18- Cristal B. & Diana S. #77

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

SL c18- Cristal B. & Diana S. #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
c3edf66
working on wave 02
cbcodee Dec 8, 2022
7ccba43
finished html index
sorindevops Dec 8, 2022
6a5b7f5
added some js and css
cbcodee Dec 8, 2022
cdc18db
updated 4 boxes still need to float on 2 x 2 matrix
sorindevops Dec 9, 2022
cda938f
fix 4 boxes to be complete
sorindevops Dec 9, 2022
e29ff66
center text
sorindevops Dec 9, 2022
3a1c81b
do we change center
sorindevops Dec 9, 2022
0451f4a
reset button function
sorindevops Dec 9, 2022
7bff157
reset button function
sorindevops Dec 9, 2022
d5cb193
reset button function
sorindevops Dec 9, 2022
8dd36d0
temp color added
cbcodee Dec 10, 2022
b82c635
added links to get
cbcodee Dec 10, 2022
9064a09
added links from proxy to requests
cbcodee Dec 10, 2022
0b3ae55
findweather
cbcodee Dec 10, 2022
f97d090
changed if statement
sorindevops Dec 10, 2022
e4de91f
Merge branch 'main' of https://github.com/cbcodee/weather-report
sorindevops Dec 10, 2022
dfee627
increase decrease working
cbcodee Dec 10, 2022
b4c8260
changed reset
sorindevops Dec 10, 2022
00bba11
change for realtime city
sorindevops Dec 10, 2022
e4c4be4
got current weather
cbcodee Dec 10, 2022
365df73
got reset button to work
cbcodee Dec 10, 2022
6a0c716
number color changes
cbcodee Dec 11, 2022
4dafa9f
changes
sorindevops Dec 11, 2022
dfbd178
Resolve conflicts
sorindevops Dec 11, 2022
e0f3320
Resolve conflicts
sorindevops Dec 11, 2022
42a8088
able to display city name
cbcodee Dec 11, 2022
d3f6c69
Merge branch 'main' of https://github.com/cbcodee/weather-report
cbcodee Dec 11, 2022
0f3fb33
added colors
cbcodee Dec 11, 2022
1fd22dd
Resolve conflicts
sorindevops Dec 11, 2022
a36e9d8
Resolve conflicts
sorindevops Dec 11, 2022
d2320fb
changes to js
sorindevops Dec 11, 2022
a3b3ca2
updated sky code
sorindevops Dec 11, 2022
3ba686f
updated sky code
sorindevops Dec 11, 2022
71efe0e
updated sky code
sorindevops Dec 11, 2022
f19f9c6
updated sky code
sorindevops Dec 12, 2022
219ae05
updateSky working
cbcodee Dec 12, 2022
c4dfabd
comments
cbcodee Dec 12, 2022
41fcea1
updated emojis
cbcodee Dec 12, 2022
68b6568
removed comments
cbcodee Dec 12, 2022
ec787d9
final
cbcodee Dec 12, 2022
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
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.vscode
.DS_Store
node_modules
.cache
.cache
.env
49 changes: 48 additions & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,59 @@

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 validate your html when possible. The errors/warnings can help us write cleaner HTML. Even if our markup looks fine in the browser we're using, there's no guarantee invalid HTML will do so on other browsers, and it could even break in the same browser since there are no layout guarantees made for invalid HTML.

<!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 href="/styles/index.css" rel="stylesheet">

</head>
<body>

<h1>WEATHER CHANNEL</h1>

<h1 id="change-city" class="diff-city">🌳 Weather for the City of <span id="cityInput">Seattle</span> 🍃</h1>

<script src="./node_modules/axios/dist/axios.min.js"></script>
Copy link

Choose a reason for hiding this comment

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

Prefer to keep you script imports together. We should put this down by the script tag importing our index.js.

<section class="temp-section">
<div id="temperature">
<h3>Temperature</h3>
<span id="up"><button>🔼</button></span>
<span id="tempNum">0</span>
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 a value for UI elements (which just happens to match the initial value backing the control), I like to leave the initial markup blank, and update the UI on first load from the initial backing values in the JS code. So like here, leave off the 0, then when the document loads, update the UI in the JS to show the initial temperature value. This would protect us from changing the default starting JS value, but having it be out of sync with what the UI is showing.

Currently, your script values assume the temperature will start at 48, so it looks like the displayed temp jumps from 0 to 49 or 47 (depending on the button pushed).

I would tend to do this for any of the configurable elements (temperature, ground, sky, city).

<span id="down"><button>🔽</button></span>
<button id="currentWeather">Get real time weather</button>
</div>
</div>
Copy link

Choose a reason for hiding this comment

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

This is an extra closing div which breaks validation.

</section>
<section class="sky-section">
<div id="skyOptions">
<h3>Sky</h3>
<select name="sky-weather" id="climate">
<option>sunshine</option>
<option>rain</option>
<option>snow</option>
<option>clouds</option>
<option>wind</option>
</select>
</div>
</section>


<section class="city_section">
<div id="city-name">
<h3>City</h3>
<input id="newCity" type="text">
<button id="reset" class="reset-button">Reset</button>
</div>
</section>

<section class="emojis">
<h3>Weather Visual</h3>
<div id="weather-emojis"></div>
<div id="emojis-below"> 🌴__🐍_🦂_🌴🌵__🐍_🏜_🦂 </div>

</section>

</body>
<script src="src/index.js"></script>
Copy link

Choose a reason for hiding this comment

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

script tags must appear either in the body or head. So this should be up one line.

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

const state = {
city: 'Seattle',
lat: 47.6038321,
long: -122.3300624,
temp: 48,
};

const convertKtoF = (temp) => {
return (temp - 273.15) * (9 / 5) + 32;
};

// increase/decrease
const upButton = document.getElementById('up');
Copy link

Choose a reason for hiding this comment

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

Because the script tag importing this file appears towards the end of the body (currently after, but it should be inside), these lookups and subsequent event registration should all succeed. But a more general approach would be to put the lookup code in a "load elements" function of some kind, and the event registration in a "register events" function, and call them in a handler registered for the DOMContentLoaded event.

const downButton = document.getElementById('down');
let display = document.getElementById('tempNum');
Copy link

Choose a reason for hiding this comment

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

Prefer const to let


upButton.addEventListener('click', function increaseTemp() {
state.temp++;
display.textContent = state.temp;
Copy link

Choose a reason for hiding this comment

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

This line is currently also being done as part of displayEmojis. Consider extracting it to a helper function,

const refreshTemp = () => {
  display.textContent = state.temp;
};

And packaging it up with displayEmojis to represent the actions needed to update the UI when the temperature changes

const updateTempUI = () => {
  refreshTemp();
  displayEmojis();
};

displayEmojis();
});
downButton.addEventListener('click', function decreaseTemp() {
state.temp--;
display.textContent = state.temp;
displayEmojis();
});

const findLatAndLong = () => {
axios
Copy link

Choose a reason for hiding this comment

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

We should always return any promise chains we create in helper functions (axios.get starts a promise chain) so that additional actions could be chained onto the end after the function logic completes.

  return axios.get(...).then(...)

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

Consider creating a global variable to hold the server part of the address you're contacting (the base URL), then use that variable in your API call addresses (such as by using template strings). If you need to point your endpoints elsewhere (such as when deploying), this can make it much easier to ensure everything is updated together.

params: {
q: state.city,
},
})
.then((response) => {
state.lat = response.data[0].lat;
state.long = response.data[0].lon;
findWeather();
Comment on lines +38 to +40
Copy link

Choose a reason for hiding this comment

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

Rather than stashing the lat/lon and then calling the weather api call directly, consider returning the lat/lon from this .then callback so that it would be available to act as the input to a subsequent .then callback. We could then make a separate function whose responsibility is to combine the city->lat/lon function with the lat/lon->weather function.

Assuming findLatAndLong was a function taking a city name and returning a promise to an object shaped like {lat: 0, lon: 0}, and findWeather was a function taking lat and lon values, and returning a promise to a temperature, we could write a third function findWeatherForCity as follows:

const findWeatherForCity = (cityName) => {
  return findLatAndLong(cityName)
    .then(({lat, lon}) => {
      return findWeather(lat, lon);
    })
};

We could then use this as

findWeatherForCity(state.city)
  .then(temp => {
      // code taken from findWeather below
      state.temp = temp;
      // using the method suggested above to update the UI
      updateTempUI();
  })

});
};

const currentWeather = document.getElementById('currentWeather');
currentWeather.addEventListener('click', findLatAndLong);

const findWeather = (lat, long) => {
axios
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 here as mentioned above.

.get('http://127.0.0.1:5000/weather', {
params: {
lat: state.lat,
lon: state.long,
Comment on lines +51 to +52
Copy link

Choose a reason for hiding this comment

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

The app doesn't need to track the lat/lon over time. It's only used during the chained city/weather call. So rather than stashing those values in the state and using them here, use the parameters that you gave this function (just be sure to pass in values where it's called).

// units: imperial,
},
})
.then((response) => {
const weather = response.data;
console.log(weather);
state.temp = Math.round(convertKtoF(weather.main.temp));
displayEmojis();
Comment on lines +59 to +60
Copy link

Choose a reason for hiding this comment

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

Rather than stashing the temperature result directly in the application state here, consider returning the temperature from the .then callback. This would make it available to a subsequent .then callback that could be chained outside this helper. In that case, we also need to be sure to return the end of the promise chain itself from the helper, so that additional actions can be chained to run after this helper completes.

// console.log(weather);
});
// console.log(findWeather);
};

currentWeather.addEventListener('click', findWeather);

const updateCity = (event) => {
const newCity = event.target.value;
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 event supplied by the browser.

state.city = newCity;
displayCity();
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.

I like how this is set up to update the state value for the city, and then use the displayCity function, whose responsibility is to update the UI to reflect the current city state. Consider extracting these lines into a helper setCity(newCity) that you could also use in the reset handler.

};

const newCity = document.getElementById('newCity');
newCity.addEventListener('input', updateCity);
const resetText = () => {
const newCity = document.getElementById('newCity');
newCity.value = 'Seattle';
updateCity();
Copy link

Choose a reason for hiding this comment

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

updateCity expects to be registered as an event handler (it has a parameter to receive event details). Calling it here without event information leads to an error when trying to update the city in the UI. If we had a setCity helper as described above, we could use that instead, passing in the reset default city value.

};
// Get the button
let resetButton = document.getElementById('reset');
// Add a click event listener to the button
resetButton.addEventListener('click', resetText);

const displayCity = () => {
document.getElementById('cityInput').textContent = state.city;
};

const displayEmojis = () => {
let numColor = 'red';
let emojisBelow = '🌵__🐍_🦂_🌵🌴__🐍_🏜_🦂';
if (state.temp > 80) {
Copy link

Choose a reason for hiding this comment

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

Notice the repetition in these if/else if blocks. Code like this tends to be finicky, since humans tend to make easily overlooked typos that can be hard to track down. Consider a data structure and accompanying code similar to the following:

const tempColors = [
  [80, {ground: 'ground emojis', color: 'temp color'}],
  [70, {ground: 'ground emojis', color: 'temp color'}],
  [60, {ground: 'ground emojis', color: 'temp color'}],
  [50, {ground: 'ground emojis', color: 'temp color'}],
  [null, {ground: 'ground emojis', color: 'temp color'}],
];

const getSettingsForTemp = (temp) => {
  for (const [boundaryTemp, tempSettings] of tempColors) {
    if (boundaryTemp === null || temp > boundaryTemp) {
      return tempSettings;
    }
  }
};

Looking for repetition in the structure of our code and refactoring it to be captured in a data structure instead can make our code more flexible (behavior can be changed solely by changing data) while simplifying the code working with the data.

If we had this function, how could we simplify the logic here in displayEmojis?

emojisBelow = '🌴__🐍_🦂_🌴🌵__🐍_🏜_🌴';
numColor = 'red';
} else if (state.temp > 70) {
emojisBelow = '🌸🌿🌼__🌷🌻🦋_☘️🌱_🦋🌷';
numColor = 'orange';
} else if (state.temp > 60) {
emojisBelow = '🥾🏞️🏕🧗🚵⛰_🪨🥾🏞️🏕🧗🚵⛰';
numColor = 'green';
} else if (state.temp > 50) {
emojisBelow = '🍂☕️🪵🍂☕️🪵🍂☕️🪵🍂☕️🪵🍂☕️🪵';
numColor = 'purple';
} else {
emojisBelow = '🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲';
numColor = 'grey';
}
const newEmojis = document.getElementById('emojis-below');
newEmojis.textContent = emojisBelow;

const temperature = document.getElementById('tempNum');
temperature.className = numColor;
Copy link

Choose a reason for hiding this comment

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

Nice approach of setting a css class with the style details rather than setting the style properties in the code here itself.

temperature.textContent = String(state.temp);
};

const updateSky = () => {
Copy link

Choose a reason for hiding this comment

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

The comments about splitting the function responsibility for temp/color would also apply to the sky function here. To think about how we could make this more data-driven, consider what changes we might make to this function if we had a data structure resembling:

const skySettings = {
  clouds: 'clouds emoji',
  sunshine: 'sunshine emoji',
  rain: 'rain emoji',
  snow: 'snow emoji',
  wind: 'wind emoji',
};

const inputSky = document.getElementById('climate').value;
let sky = '';
if (inputSky === 'clouds') {
sky = '☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️';
} else if (inputSky === 'sunshine') {
sky = '☁️☁️☁️ ☁️ ☁️ ☀️ ☁️ ☁️☁️☁️';
} else if (inputSky === 'rain') {
sky = '🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧';
} else if (inputSky === 'snow') {
sky = '🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨';
} else if (inputSky === 'wind') {
sky = '🌬️☁️🌬️☁️🌬️☁️🌬️☁️🌬️☁️🌬️☁️';
}

const skyContainer = document.getElementById('weather-emojis');
skyContainer.textContent = sky;
};

updateSky();
const chooseSky = document.getElementById('climate');
chooseSky.addEventListener('change', updateSky);

console.log(updateSky);
75 changes: 75 additions & 0 deletions styles/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@

* {
box-sizing: border-box;
Copy link

Choose a reason for hiding this comment

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

👍 border-box breaks my brain much less than the default content-box rules.

}

body {
/* margin: 0; */
display: flex;
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 flex along with specified element widths to achieve your four-square layout.

flex-wrap: wrap;
background-color: #cad0bd;
}


.temp-section {
background: yellow;
float: left;
width: 50%;
height: 400px;
text-align: center;
/* padding-bottom: 50%; */
}

.sky-section{
background: lightblue;
float: right;
width: 50%;
height: 400px;
text-align: center;
}

.city_section{
background: orange;
float: right;
width: 50%;
text-align: center;
}

.emojis {
background: rgb(121, 194, 121);
float: left;
width: 50%;
height: 400px;
text-align: center;
}


#tempNum {
font-size: 3rem;

}

.red {
color: red;
}

.orange {
color: orange;
}

.yellow {
color: gold;
}

.green {
color: green;
}

.purple {
color: purple
}

.grey {
color: grey;
}