Jamie Horvath C-17 Digital Sharks#87
Open
glassofcat wants to merge 1 commit into
Open
Conversation
audreyandoy
reviewed
Jul 20, 2022
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Fantastic work Jamie! Your project met all the requirements. And I loved your algorithm to generate the hex color as the temp changes definitely made up for it.
I didn't have much feedback to add as your code is very neat. Design-wise, it wasn't obvious that users could click on the city text to switch to 'Reset Location'. A hover effect or a separate button that clearly says "reset city" would have been helpful.
Other than that great work!
Grade: 🟢
Comment on lines
+41
to
+69
| <!-- Wave 2: | ||
| lets do centered column flex elements | ||
| reset button0<(city name) | ||
| 3 buttons stacked next to <(70 degrees) | ||
| update the background gradient per temperature | ||
|
|
||
| x An element that displays the temperature | ||
| x A clickable element to increase temperature | ||
| x A clickable element to decrease temperature | ||
| x An element that displays a landscape | ||
| x bottom gradient from blue to red | ||
|
|
||
| Wave 3: | ||
|
|
||
| x An element that displays the city name | ||
| x? An element that contains an <input type="text"> element, used to rename the city | ||
|
|
||
| Wave 4: | ||
|
|
||
| x A clickable element to get the current temperature of the displayed city name | ||
|
|
||
| Wave 5: | ||
| top gradient yellow sun, grey cloud, blue rain, white snow | ||
| ? A <select> dropdown element to set the sky type | ||
| ? An element that displays a sky | ||
|
|
||
| Wave 6: | ||
|
|
||
| x A clickable element to reset the city name --> |
There was a problem hiding this comment.
These are great notes, however, the intended use of PRs is to share code that we'd like our team to also have. I highly recommend putting these type of notes in a local branch as you're working through some code.
Comment on lines
+12
to
+32
| const getTemp = () => { | ||
| axios | ||
| .get('http://localhost:5000/weather', { | ||
| params: { | ||
| lat: state.lat, | ||
| lon: state.lon, | ||
| }, | ||
| }) | ||
| .then((response) => { | ||
| state.temp = parseInt( | ||
| ((response.data['current'].temp - 273) * 9) / 5 + 32 | ||
| ); | ||
| state.weather = response.data['current']['weather'][0].main; | ||
| console.log('success in finding weather!', state.temp, state.weather); | ||
| tempChange(state.temp, true); | ||
| }) | ||
| .catch((error) => { | ||
| console.log('error in weather'); | ||
| }); | ||
| return state.temp; | ||
| }; |
Comment on lines
+4
to
+10
| const state = { | ||
| location: '', | ||
| temp: 70, | ||
| weather: '', | ||
| lat: 0, | ||
| lon: 0, | ||
| }; |
Comment on lines
+37
to
+38
| const toHex = (dec) => (dec < 16 ? '0' + dec.toString(16) : dec.toString(16)); | ||
| const normalize = (newTemp) => (newTemp % 35) / 35; |
There was a problem hiding this comment.
LOVE this formula to generate the temp color!!!
| } else { | ||
| color = ['FF', '00', '00']; | ||
| } | ||
| return ['#', ...color].join(''); |
| }; | ||
| ///////////////////////////////////////////// | ||
| const getLocation = () => { | ||
| print; |
Comment on lines
+110
to
+113
| console.log('success in finding location!', state.lat, state.lon); | ||
| }) | ||
| .catch((error) => { | ||
| console.log('error in location'); |
There was a problem hiding this comment.
Good job in error handling and utilizing logs to confirm the behavior of your get request. However, unless we're making a terminal program, we should keep console.logs out of PRs.
Comment on lines
+118
to
+126
| const locationChange = () => { | ||
| const currentLocation = document.querySelector('#location'); | ||
| const locationButton = document.querySelector('#locationReset'); | ||
|
|
||
| state.location = currentLocation.value; | ||
| getLocation(); | ||
| locationButton.textContent = state.location; | ||
| currentLocation.value = ''; | ||
| }; |
Comment on lines
+128
to
+134
| const clearLocation = () => { | ||
| const locationButton = document.querySelector('#locationReset'); | ||
| locationButton.textContent = 'Reset Location'; | ||
| state.location = ''; | ||
| state.lat = 0; | ||
| state.lon = 0; | ||
| }; |
Comment on lines
+136
to
+154
| const registerEventHandlers = () => { | ||
| const upTempButton = document.querySelector('#up_temp'); | ||
| upTempButton.addEventListener('click', (event) => tempChange(1, false)); | ||
| const downTempButton = document.querySelector('#down_temp'); | ||
| downTempButton.addEventListener('click', (event) => tempChange(-1, false)); | ||
| const setTempButton = document.querySelector('#lookup_temp'); | ||
| setTempButton.addEventListener('click', (event) => getTemp()); | ||
|
|
||
| const updateLocation = document.querySelector('#location'); | ||
| updateLocation.addEventListener('change', (event) => locationChange()); | ||
| const updateWeather = document.querySelector('#weatherBox'); | ||
| updateWeather.addEventListener('change', (event) => weatherChange()); | ||
|
|
||
| const locationResetButton = document.querySelector('#locationReset'); | ||
| locationResetButton.addEventListener('click', (event) => clearLocation()); | ||
| }; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', registerEventHandlers); | ||
| tempChange(0, false); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.