Weather Report - Mumina, Priscilla C24#17
Conversation
…rn temp and weather, and refactors change sky
git push ;
There was a problem hiding this comment.
Pull request overview
This PR implements a weather report application for the "magic city" with interactive clothing selection and real-time weather data integration.
Key Changes:
- Implements drag-and-drop clothing selection interface based on weather conditions
- Integrates real-time weather API with temperature and sky condition controls
- Adds comprehensive CSS styling with custom fonts and responsive layout
Reviewed changes
Copilot reviewed 3 out of 37 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/index.js | Core application logic with state management, drag-and-drop functionality, and weather API integration |
| styles/index.css | Complete styling for weather app including layout, colors, and draggable clothing items |
| index.html | HTML structure with weather controls, clothing display, and city input |
| package-lock.json | Dependency lock file for ESLint and related tooling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Function to get temperature from OpenWeather API | ||
| const getWeather = async (lat, lon) => { | ||
| const response = await axios.get('http://127.0.0.1:5000//weather', { |
There was a problem hiding this comment.
Double slashes in URL path. The URL should be 'http://127.0.0.1:5000/weather' (single slash after port).
| @@ -0,0 +1,539 @@ | |||
| 'strict mode'; | |||
There was a problem hiding this comment.
Incorrect use of 'strict mode'. The correct syntax is 'use strict'; (not 'strict mode';).
| height: 130px; | ||
| } | ||
|
|
||
| .sandls { |
There was a problem hiding this comment.
Typo in class name: "sandls" should be "sandals".
| <img id='winter-boot-2' class="draggable-clothing shoes winter-boots" src="ada-project-docs/assets/winter-weather/winter-boot.png" alt="winter boot"> | ||
| <img id='rain-boot-1' class="draggable-clothing shoes rain-boots" src="ada-project-docs/assets/rainy-weather/rain-boot.png" alt="rain boot"> | ||
| <img id='rain-boot-2' class="draggable-clothing shoes rain-boots flipped" src="ada-project-docs/assets/rainy-weather/rain-boot.png" alt="rain boot"> | ||
| <img id='sandle-1' class="draggable-clothing shoes sandls" src="ada-project-docs/assets/super-hot-weather/sandal.png" alt="sandle"> |
There was a problem hiding this comment.
Typo in alt text: "sandle" should be "sandal".
| <img id='rain-boot-1' class="draggable-clothing shoes rain-boots" src="ada-project-docs/assets/rainy-weather/rain-boot.png" alt="rain boot"> | ||
| <img id='rain-boot-2' class="draggable-clothing shoes rain-boots flipped" src="ada-project-docs/assets/rainy-weather/rain-boot.png" alt="rain boot"> | ||
| <img id='sandle-1' class="draggable-clothing shoes sandls" src="ada-project-docs/assets/super-hot-weather/sandal.png" alt="sandle"> | ||
| <img id='sandle-2' class="draggable-clothing shoes sandls flipped" src="ada-project-docs/assets/super-hot-weather/sandal.png" alt="sandle"> |
There was a problem hiding this comment.
Typo in alt text: "sandle" should be "sandal".
|
|
||
| // Function to get coordinates from city name using LocationIQ | ||
| const getCoordinates = async () => { | ||
| const response = await axios.get('http://127.0.0.1:5000//location', { |
There was a problem hiding this comment.
Double slashes in URL path. The URL should be 'http://127.0.0.1:5000/location' (single slash after port).
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
| const convertKtoF = e => { | ||
| return 1.8 * (e - 273.15) + 32; | ||
| }; |
There was a problem hiding this comment.
Prefer a more descriptive variable name, such as degK.
There was a problem hiding this comment.
The ada-project-docs/assets was intended to hold resources for the README. There was a standalone folder assets that could be used for resources for your site.
There was a problem hiding this comment.
Rather than placing these resources under ada-project-docs, it would be more appropriate to use the standalone assets folder.
There was a problem hiding this comment.
👍 Nice wireframe. Having an overview to work towards can make it easier to be on the same page about your development plan.
| elements.gifFire = document.createElement('img'); | ||
| elements.gifFire.src = 'ada-project-docs/assets/landscape/fireTemp.gif'; | ||
| elements.gifFire.alt = 'A gif image of fire'; | ||
| elements.gifFire.style.display = 'block'; | ||
| elements.gifFire.style.height = '400px'; | ||
| elements.gifFire.style.width = '43.1rem'; | ||
| elements.gifFire.style.position = 'absolute'; | ||
| elements.gifFire.style.bottom = '90px'; | ||
| elements.gifFire.style.zIndex = '0'; |
There was a problem hiding this comment.
Rather than building these elements in code, you could take an approach more like what you did for the clothing elements. That would allow the main styling to be applied through CSS rather than hard-coding the style. A way to be able to show or conceal the element is to default it to display: none; and then set it to display: block; when it needs to be shown.
| const updateTemp = (event) => { | ||
| state.temp = event.target.value; | ||
| state.tempLabel = event.target.value; | ||
| elements.tempLabel.textContent = event.target.value; | ||
| }; | ||
|
|
||
| const handleTempSlider = (event) => { | ||
| updateTemp(event); | ||
| changeBackgroundColor(); | ||
| updateLandscape(); | ||
| }; |
There was a problem hiding this comment.
Nice set of functions to handle and propagate changes to the temperature value. There are a variety of ways we can carve up the logic for changes like this. One way is to keep the event-aware logic solely in the function registered as the event handler (its responsibility is to extract any information from the event, and pass it along). Then we can have a function that updates the application state (its responsibility is state mutation), and then one final fiunction to "publish" the state value to the rest of the UI. Something like this:
const publishTemp = () => {
elements.tempLabel.textContent = state.temp.toString();
changeBackgroundColor();
updateLandscape();
};
const updateTemp = (newTemp) => {
state.temp = newTemp;
};
const handleTempSlider = (event) => {
const newTemp = event.target.value;
updateTemp(newTemp);
publishTemp();
};Sometime the mutator function is also responsible for propogating the value to the UI, but often, we like to have a way to just update the state alone so that if there are multiple updates to the state, those can all occur with only a single "publish" operation after they've all happened.
This line of thought can be applied to pretty much all of the UI event handlers.
| state.city = event.target.value; | ||
| }; | ||
|
|
||
| const handleRealTimeTemp = async () => { |
There was a problem hiding this comment.
Nice function to combine the operations of the two API calls. We could go a step further and create a third function that solely combines those two calls into a function that gets the weather for a place name. Then a final function could call the wrapper function, attaching the application-specific logic in the final functions, allowing the API functions to be independent of the application.
| // Function to get coordinates from city name using LocationIQ | ||
| const getCoordinates = async () => { | ||
| const response = await axios.get('http://127.0.0.1:5000//location', { | ||
| params: { q: `${state.city}`} }); |
There was a problem hiding this comment.
Rather than having this function need to know to get the city name from the state, prefer to pass that in as a function parameter (as you did for the weather location helper!). That would allow this function to be fully independent of the application-specific logic, being more reusable and testable.
| <div id="slider-bar"> | ||
| <label for="temp">Temp</label> | ||
| <button id="realtime-temp">Get Realtime Temperature</button> | ||
| <input type="range" id="temp" name="temperature" max="200"> |
There was a problem hiding this comment.
Neat use of a slider rather than individual buttons.
One thing to be careful of with a slider is that its possible to have a slider that's too narrow to give the user precise control over the value. E.g., if the width of the slider is 50 pixels, but the user needs to be able to set a range of 100 values, the slider is likely to need to skip over values.
| @@ -0,0 +1,539 @@ | |||
| 'strict mode'; | |||
There was a problem hiding this comment.
It looks like GitHub has turned on at least some degree of Copilot auto review. All of its suggestions appear correct to me, so be sure to review those as well.
No description provided.