-
Notifications
You must be signed in to change notification settings - Fork 88
Sea Turtles - Tiffini H. #68
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
4110865
9234f70
5e60ae6
9bd9fd9
4fc3bb1
3d5e109
9ce91ac
5cc7d3f
83a4ac4
290bdbe
f4264a7
ee1eff2
dd3ffcc
b8c0983
863546f
67daa00
8f1054e
74ca9d0
fa34da4
e3acec5
65e1872
e389361
37d82d0
859d189
ac98a79
e7ef4f0
11be2c8
32433a1
7aa31d1
777f9cc
bff3e7a
dbb06b7
906bc1c
6237956
377af4b
71e2c0b
de63577
64c0980
710943a
3e6a175
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,8 +5,70 @@ | |
| <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> | ||
| <div class="container"> | ||
| <header> | ||
| <img | ||
| id="cloud-logo" | ||
| src="assets/cloud-logo.png" | ||
| alt="A small, white cloud outlined in black"> | ||
| <h1 id="weather-wizard">weather wizard</h1> | ||
| </header> | ||
|
|
||
| <section id="current-city"> | ||
| <h2>The temperature in <span id="display-city">Chicago</span> is:</h2> | ||
| </section> | ||
|
|
||
| <button class="temp-adjusters" id="decrease-temp">-</button> | ||
| <h2 id="temperature">0</h2> | ||
| <button class="temp-adjusters" id="increase-temp">+</button> | ||
|
|
||
| <section id="city-search"> | ||
| <label for="input-city"><h2>Show me the weather in:</h2></label> | ||
| <input type="text" id="input-city" name="name"> | ||
| </section> | ||
|
|
||
| <section id="reset-city-or-temp"> | ||
| <button class="reset-button" id="reset-city-button"><h2>RESET CITY</h2></button> | ||
| <button class="reset-button" id="current-temp-button"><h2>SHOW CURRENT TEMPERATURE</h2></button> | ||
| </section> | ||
|
|
||
| <section id="sky-and-landscape-container"> | ||
| <figure id="sky-with-caption"> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of the |
||
| <img | ||
| id="sky-image" | ||
| src="assets/sky/grooveland-designs-sunny.jpg" | ||
| alt="The sun, off in the distance, surrounded by bright blue sky"> | ||
| <figcaption id="sky-caption">Photo by Grooveland Designs</figcaption> | ||
| </figure> | ||
|
|
||
| <div id="sky-selector"> | ||
| <label for="sky-dropdown"><h2>CHANGE SKY</h2></label> | ||
| <select name="skies" id="sky-dropdown"> | ||
| <option value="">Please select an option below</option> | ||
| <option value="sunny">Sunny</option> | ||
| <option value="cloudy">Cloudy</option> | ||
| <option value="stormy">Stormy</option> | ||
| <option value="snowy">Snowy</option> | ||
| </select> | ||
| </div> | ||
|
|
||
| <figure id="landscape-with-caption"> | ||
| <img | ||
| id="landscape-image" | ||
| src="assets/marissa-rodriguez-summer.jpg" | ||
| alt="Crystal clear turquoise water in a pool or sea"> | ||
| <figcaption id="landscape-caption">Photo by Marissa Rodriguez</figcaption> | ||
| </figure> | ||
| </section> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall, nice division of content into sections.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! I worked really hard to make my html as thoughtful as possible based on your feedback about how HTML can impact accessibility, so I'm glad it's an improvement! |
||
|
|
||
| <footer>© 2022 Tiffini Hyatt</footer> | ||
| </div> | ||
|
|
||
| <script src="./node_modules/axios/dist/axios.min.js"></script> | ||
| <script src="src/index.js" type="text/javascript"></script> | ||
| </body> | ||
| </html> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,238 @@ | ||
| 'use strict'; | ||
|
|
||
| const state = { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of the state object! |
||
| temperature: 0, | ||
| coordinates: [], | ||
| currentColor: 'black', | ||
| currentLandscape: 'summer', | ||
| currentCity: 'Chicago', | ||
| currentSky: 'sunny', | ||
| }; | ||
|
|
||
| // increase temperature | ||
| const increaseTemperature = () => { | ||
| const temperatureContainer = document.getElementById('temperature'); | ||
| state.temperature += 1; | ||
|
|
||
| temperatureContainer.textContent = `${state.temperature}° F`; | ||
|
|
||
| changeTemperatureColor(); | ||
| changeLandscape(); | ||
| changeSky(); | ||
| }; | ||
|
|
||
| // decrease temperature | ||
| const decreaseTemperature = () => { | ||
| const temperatureContainer = document.getElementById('temperature'); | ||
| state.temperature -= 1; | ||
|
|
||
| temperatureContainer.textContent = `${state.temperature}° F`; | ||
|
|
||
| changeTemperatureColor(); | ||
| changeLandscape(); | ||
| changeSky(); | ||
|
Comment on lines
+31
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really nice use of helper functions to divide up the actions we want to take into smaller portions that are easier to read and test. |
||
| }; | ||
|
|
||
| // helper function to change temperature text color | ||
| const changeTemperatureColor = () => { | ||
| const temperatureContainer = document.getElementById('temperature'); | ||
|
|
||
| if (state.temperature >= 80) { | ||
| state.currentColor = 'coral'; | ||
| } else if (state.temperature >= 60 && state.temperature <= 79) { | ||
| state.currentColor = '#f5b942'; | ||
| } else if (state.temperature >= 40 && state.temperature <= 59) { | ||
| state.currentColor = 'teal'; | ||
| } else if (state.temperature >= 20 && state.temperature <= 39) { | ||
| state.currentColor = '#1a557d'; | ||
| } else { | ||
| state.currentColor = '#67686b'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In production code we'd likely want to include comments about what each of the hex colors are, since many folks cannot sight-read hex values.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, that makes sense. Thank you! |
||
| } | ||
|
|
||
| temperatureContainer.style.color = state.currentColor; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By setting the So in our CSS we'd have rule sets: ...
.coral {
color: coral;
}
.teal {
color: teal;
}
... /* (and so on for the other colors necessary) */And here we could do something like: ...
if (state.temp >= 80) {
currentTemp.className = 'coral';
... |
||
| }; | ||
|
|
||
| // helper function to change landscape based on temp | ||
| const changeLandscape = () => { | ||
| const landscapeImage = document.getElementById('landscape-image'); | ||
| const landscapeCaption = document.getElementById('landscape-caption'); | ||
|
|
||
| if (state.temperature >= 80) { | ||
| landscapeImage.src = 'assets/marissa-rodriguez-summer.jpg'; | ||
| landscapeImage.alt = 'Crystal clear turquoise water in a pool or sea'; | ||
| landscapeCaption.textContent = 'Photo by Marissa Rodriguez'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like how you handled attribution for the photos! |
||
| } else if (state.temperature >= 60 && state.temperature <= 79) { | ||
| landscapeImage.src = 'assets/laura-adai-spring.jpg'; | ||
| landscapeImage.alt = 'A close up image of a branch with pink flowers'; | ||
| landscapeCaption.textContent = 'Photo by Laura Adai'; | ||
| } else if (state.temperature >= 40 && state.temperature <= 59) { | ||
| landscapeImage.src = 'assets/janmesh-shah-fall.jpg'; | ||
| landscapeImage.alt = 'A tree full of golden leaves'; | ||
| landscapeCaption.textContent = 'Photo by Janmesh Shah'; | ||
| } else { | ||
| landscapeImage.src = 'assets/donnie-rosie-winter.jpg'; | ||
| landscapeImage.alt = | ||
| 'A forest with tall, snowcapped trees above a bed of snow'; | ||
| landscapeCaption.textContent = 'Photo by Donnie Rosie'; | ||
| } | ||
| }; | ||
|
Comment on lines
+56
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we have large if-statements like this where each if-block performs the same operations with slightly different values, it can be worth making a helper function that handles the updates to reduce places we're repeating code and could accumulate small typos. With the helper function, each of the ifs here would call that function passing the desired parameters rather than setting attributes themselves. const changeLandscape = () => {
...
} else if (state.temperature >= 40 && state.temperature <= 59) {
updateLandscapeImage(
'assets/janmesh-shah-fall.jpg',
'A tree full of golden leaves',
'Photo by Janmesh Shah'
)
} else {
updateLandscapeImage(
'assets/donnie-rosie-winter.jpg',
'A forest with tall, snowcapped trees above a bed of snow',
'Photo by Donnie Rosie'
)
}
...const updateLandscapeImage = (imagePath, altText, caption) => {
landscapeImage.src = imagePath;
landscapeImage.alt = altText;
landscapeCaption.textContent = caption;
} |
||
|
|
||
| // helper function to change sky based on temp | ||
| const changeSky = () => { | ||
| const skyImage = document.getElementById('sky-image'); | ||
| const skyCaption = document.getElementById('sky-caption'); | ||
|
|
||
| if (state.temperature >= 80) { | ||
| skyImage.src = 'assets/sky/grooveland-designs-sunny.jpg'; | ||
| skyImage.alt = | ||
| 'The sun, off in the distance, surrounded by bright blue sky'; | ||
| skyCaption.textContent = 'Photo by Grooveland Designs'; | ||
| } else if (state.temperature >= 60 && state.temperature <= 79) { | ||
| skyImage.src = 'assets/sky/brandon-morgan-lightning.jpg'; | ||
| skyImage.alt = | ||
| 'A dark sky with one big bolt of lightning stretching toward the ground'; | ||
| skyCaption.textContent = 'Photo by Brandon Morgan'; | ||
| } else if (state.temperature >= 40 && state.temperature <= 59) { | ||
| skyImage.src = 'assets/sky/daoudi-aissa-cloudy.jpg'; | ||
| skyImage.alt = 'An overcast sky full of fluffy clouds, some ominous'; | ||
| skyCaption.textContent = 'Photo by Daoudi Aissa'; | ||
| } else { | ||
| skyImage.src = 'assets/sky/jessica-fadel-snowing.jpg'; | ||
| skyImage.alt = | ||
| 'A snow flurry against a dark background, as though it is nighttime'; | ||
| skyCaption.textContent = 'Photo by Jessica Fadel'; | ||
| } | ||
| }; | ||
|
|
||
| // manually change sky using dropdown | ||
| const manuallyChangeSky = () => { | ||
| const skyImage = document.getElementById('sky-image'); | ||
| const skyCaption = document.getElementById('sky-caption'); | ||
| const selectedSky = document.getElementById('sky-dropdown').value; | ||
|
|
||
| state.currentSky = selectedSky; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We update
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh, that's a great catch! |
||
|
|
||
| if (selectedSky === 'sunny') { | ||
| skyImage.src = 'assets/sky/grooveland-designs-sunny.jpg'; | ||
| skyImage.alt = | ||
| 'The sun, off in the distance, surrounded by bright blue sky'; | ||
| skyCaption.textContent = 'Photo by Grooveland Designs'; | ||
| } else if (selectedSky === 'cloudy') { | ||
| skyImage.src = 'assets/sky/daoudi-aissa-cloudy.jpg'; | ||
| skyImage.alt = 'An overcast sky full of fluffy clouds, some ominous'; | ||
| skyCaption.textContent = 'Photo by Daoudi Aissa'; | ||
| } else if (selectedSky === 'stormy') { | ||
| skyImage.src = 'assets/sky/brandon-morgan-lightning.jpg'; | ||
| skyImage.alt = | ||
| 'A dark sky with one big bolt of lightning stretching toward the ground'; | ||
| skyCaption.textContent = 'Photo by Brandon Morgan'; | ||
| } else if (selectedSky === 'snowy') { | ||
| skyImage.src = 'assets/sky/jessica-fadel-snowing.jpg'; | ||
| skyImage.alt = | ||
| 'A snow flurry against a dark background, as though it is nighttime'; | ||
| skyCaption.textContent = 'Photo by Jessica Fadel'; | ||
| } | ||
| }; | ||
|
Comment on lines
+108
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth creating a structure that defines the values that get reused between const skySettings = {
sunny: {
src: 'assets/sky/grooveland-designs-sunny.jpg',
alt: 'The sun, off in the distance, surrounded by bright blue sky',
caption: 'Photo by Grooveland Designs',
},
cloudy: {
src: ''assets/sky/daoudi-aissa-cloudy.jpg',
alt: 'An overcast sky full of fluffy clouds, some ominous',
caption: 'Photo by Daoudi Aissa',
}
... // And so on for the other possible skies
}Then here we could do something like: const manuallyChangeSky = () => {
const selectedSky = document.getElementById('sky-dropdown').value;
state.currentSky = selectedSky;
const skyImage = document.getElementById('sky-image');
const skyCaption = document.getElementById('sky-caption');
skyImage.src = skySettings[selectedSky].src;
skyImage.alt = skySettings[selectedSky].alt;
skyCaption.textContent = skySettings[selectedSky].caption;
};And we would be able to use the values in |
||
|
|
||
| const updateDisplayCity = () => { | ||
| const displayCity = document.getElementById('display-city'); | ||
| const userInputCity = document.getElementById('input-city').value; | ||
|
|
||
| // update display city text content | ||
| displayCity.textContent = userInputCity; | ||
| }; | ||
|
|
||
| // function to display real time temperature | ||
| const showCurrentTemp = () => { | ||
| // update state current city based on completed user input | ||
| state.currentCity = document.getElementById('display-city').textContent; | ||
|
|
||
| const currentTemp = document.getElementById('temperature'); | ||
|
|
||
| // update display temp, sky, and landscape after retrieving weather data | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of comments throughout! |
||
| getCurrentTemp().then(() => { | ||
| currentTemp.textContent = `${state.temperature}° F`; | ||
|
|
||
| changeTemperatureColor(); | ||
| changeLandscape(); | ||
| changeSky(); | ||
| }); | ||
| }; | ||
|
|
||
| // reset display city | ||
| const resetDisplayCity = () => { | ||
| const displayCity = document.getElementById('display-city'); | ||
|
|
||
| document.getElementById('input-city').value = ''; | ||
| displayCity.textContent = 'Chicago'; | ||
| state.currentCity = displayCity.textContent; | ||
|
|
||
| showCurrentTemp(); | ||
| }; | ||
|
|
||
| const registerEventHandlers = () => { | ||
| const increaseTempButton = document.getElementById('increase-temp'); | ||
| increaseTempButton.addEventListener('click', increaseTemperature); | ||
|
|
||
| const decreaseTempButton = document.getElementById('decrease-temp'); | ||
| decreaseTempButton.addEventListener('click', decreaseTemperature); | ||
|
Comment on lines
+174
to
+178
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since const setTemperature = (changeInTemp) => {
state.temperature += changeInTemp;
const temperatureContainer = document.getElementById('temperature');
temperatureContainer.textContent = `${state.temperature}° F`;
changeTemperatureColor();
changeLandscape();
changeSky();
};Then here where we register the handlers, we could call our new function with a parameter by doing so inside an anonymous function: const increaseTempButton = document.getElementById('increase-temp');
increaseTempButton.addEventListener('click', () => { setTemperature(1) });
const decreaseTempButton = document.getElementById('decrease-temp');
decreaseTempButton.addEventListener('click', () => { setTemperature(-1) }); |
||
|
|
||
| const userInputCity = document.getElementById('input-city'); | ||
| userInputCity.addEventListener('input', updateDisplayCity); | ||
|
|
||
| const currentTempButton = document.getElementById('current-temp-button'); | ||
| currentTempButton.addEventListener('click', showCurrentTemp); | ||
|
|
||
| const resetCityButton = document.getElementById('reset-city-button'); | ||
| resetCityButton.addEventListener('click', resetDisplayCity); | ||
|
|
||
| const skyDropdown = document.getElementById('sky-dropdown'); | ||
| skyDropdown.addEventListener('change', manuallyChangeSky); | ||
| }; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', registerEventHandlers); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's easy to overlook this line nestled between functions. I recommend moving it to the very bottom to keep functions together and separate from code that we immediately call and execute. |
||
|
|
||
| // endpoints to retrieve real time weather data | ||
|
|
||
| const getCurrentTemp = () => { | ||
| // get latitude and longitude of current city | ||
| return ( | ||
| axios | ||
| .get('http://127.0.0.1:5000/location', { | ||
| params: { | ||
| q: state.currentCity, | ||
| }, | ||
| }) | ||
| .then((response) => { | ||
| const lat = response.data[0]['lat']; | ||
| const lon = response.data[0]['lon']; | ||
|
|
||
| return [lat, lon]; | ||
| }) | ||
| // get current temp of current city | ||
| .then((response) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh, I'd love to talk about this when we check in next! |
||
| return axios | ||
| .get('http://127.0.0.1:5000/weather', { | ||
| params: { | ||
| lat: response[0], | ||
| lon: response[1], | ||
| }, | ||
| }) | ||
| .then((response) => { | ||
| const tempInK = response.data.current.temp; | ||
| const tempInF = Math.floor(1.8 * (tempInK - 273) + 32); | ||
|
|
||
| // update state temperature | ||
| state.temperature = tempInF; | ||
|
|
||
| return tempInF; | ||
| }) | ||
| .catch(() => { | ||
| console.log('Error retrieving current weather'); | ||
| }); | ||
| }) | ||
| .catch(() => { | ||
| console.log('Error retrieving latitude and longitude'); | ||
| }) | ||
| ); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend replacing this
divwith amainelement, sincemainis intended to represent the significant content of a page. Depending on if it significantly changes the layout, another option might be to moveclass="container"to thebodytag and remove thisdiv.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Kelsey! I thought that this div would maybe be okay since I was grouping just for styling purposes, but I can see how a
maintag would have made more sense.