Crow - Lexy & Possum - Wenxin by Likhoram · Pull Request #1 · Ada-C24/weather-report

Likhoram · 2025-12-04T22:30:10Z

No description provided.

Copilot

Pull request overview

This PR implements a weather report application for students Lexy and Wenxin. The implementation includes a user interface for displaying and controlling temperature, sky conditions, and city names, with integration to a weather API proxy server.

Key changes:

Implemented complete weather report UI with temperature controls, sky selection, and city name input
Added real-time weather data fetching via LocationIQ and OpenWeatherMap proxy APIs
Created responsive CSS styling with temperature-based color themes and sky-based background colors

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
styles/index.css	Complete CSS implementation with grid layout, color schemes, and responsive design elements
src/index.js	JavaScript logic for temperature controls, API integration, city management, and UI updates
index.html	HTML structure with semantic sections for temperature, sky, city name, and weather garden
package-lock.json	Auto-generated dependency lock file for ESLint and related tooling
coworking_agreement.md	Team collaboration agreement with accessibility needs, communication preferences, and signatures
README.md	Added links to project resources including slides, repositories, and live servers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-04T22:32:21Z

+//wave 3
+
+const cityName = document.getElementById("headerCityName")
+const cityInput = document.getElementById("cityNameInput")


Missing semicolon after variable declaration. For consistency with the rest of the codebase, add a semicolon at the end of this line.

Suggested change

const cityInput = document.getElementById("cityNameInput")

const cityInput = document.getElementById("cityNameInput");

Copilot · 2025-12-04T22:32:22Z

+
+//wave 6
+const defaultCity = "Seattle";
+const resetButton = document.getElementById("cityNameReset")


Missing semicolon after variable declaration. For consistency with the rest of the codebase, add a semicolon at the end of this line.

Suggested change

const resetButton = document.getElementById("cityNameReset")

const resetButton = document.getElementById("cityNameReset");

Copilot · 2025-12-04T22:32:22Z

+    text-decoration: none;
+    display: inline-block;
+    font-size: 16px;
+    border-radius: 10px


Missing semicolon at the end of this CSS rule. While not strictly required, it's best practice to include semicolons consistently to prevent potential issues when minifying or concatenating CSS files.

Suggested change

border-radius: 10px

border-radius: 10px;

Copilot · 2025-12-04T22:32:22Z

+
+//wave 3
+
+const cityName = document.getElementById("headerCityName")


Missing semicolon after variable declaration. While JavaScript automatic semicolon insertion (ASI) will handle this, it's best practice to explicitly include semicolons for code clarity and to prevent potential issues.

Suggested change

const cityName = document.getElementById("headerCityName")

const cityName = document.getElementById("headerCityName");

kelsey-steven-ada

Nice work team, awesome to play around with the deployed version ^_^ Please check out the feedback here and in Learn and let me know if you have questions or if there's anything I can clarify!

kelsey-steven-ada · 2025-12-10T18:33:26Z

+const cityName = document.getElementById("headerCityName");
+const cityInput = document.getElementById("cityNameInput");


There are a lot of variables loose in the Javascript file. For best practices, and to make code easier to read and maintain, I strongly suggest creating a central object to represent the state of the application that holds all of these things together. It can be really hard to find where something is defined when variables are interspersed with functions declared at the same level.

kelsey-steven-ada · 2025-12-10T18:36:10Z

+        <span>For the lovely city of 
+            <span id="headerCityName" class="header__city-name"></span></span>


span is much like div where it does not inherently have semantic meaning, so we want to reserve it for places where we need an inline element to wrap something for styling-purposes only. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/span

Here, we are presenting text on the screen, so to follow semantic HTML best practices, the outer element would be better represented as a <p> element with an inner <span> that only wraps the section of text that will change:

<p>For the lovely city of <span id="headerCityName" class="header__city-name"></span> </p>

updated
, thanks

kelsey-steven-ada · 2025-12-10T18:39:06Z

+                <span id="increaseTempControl">⬆️</span>
+                <span id="tempValue"></span>
+                <span id="decreaseTempControl">⬇️</span>
+            </div>
+            <button id="currentTempButton">Get Realtime Temperature</button>


I see <button> used here and for resetting the city name, but there are other interactive elements like the increase and decrease temperature buttons that are text loose in a <span>. For interactive items on the page, we typically want to use an element that indicates to a user that they can interact with it. For elements like <input> and <button>, we can nest text or an img element inside and adjust the default styling as necessary.

kelsey-steven-ada · 2025-12-10T18:47:50Z

+updateTemperatureUI();
+registerTemperatureControls();


I would like to see this file updated for organizational best practices. Outside of the global constants spread through the file, the code that initializes or sets up the UI is also not co-located. I see code that initializes the UI here, on lines 156 & 157, lines 173-174, and lines 181-184.

Thinking back to the principles of encapsulation and single responsibility, we should have a single function to group together and handle calling all of the set up code, and invoke that function one time, likely at the end of the file.

wrap them into init function

kelsey-steven-ada · 2025-12-10T18:53:15Z

+const temperatureColorClasses = [
+  'temp-hot',
+  'temp-warm',
+  'temp-mild',
+  'temp-cool',
+  'temp-cold',
+];
+
+const getTemperatureColorClass = (temp) => {
+  if (temp >= 80) {
+    return 'temp-hot';
+  }
+  if (temp >= 70) {
+    return 'temp-warm';
+  }
+  if (temp >= 60) {
+    return 'temp-mild';
+  }
+  if (temp >= 50) {
+    return 'temp-cool';
+  }
+
+  return 'temp-cold';
+};


I see that temperatureColorClasses is used when resetting the temperature to ensure the old class value gets removed, however, we are not reading the values from temperatureColorClasses here. The function getTemperatureColorClass is returning a new string. Any time we duplicate information like this, it creates an easy surface for bugs to occur due to typos, and we need to ensure these names are kept in sync any time there are updates.

How could you update the code to use temperatureColorClasses as a single source of truth for the color class names across the file?

using index to locate class

kelsey-steven-ada · 2025-12-10T20:03:09Z

+
+const currentTempButton = document.getElementById('currentTempButton');
+
+const handleCurrentTempClick = async () => {


We should be consistent with syntax across a file, is there a reason to declare this function in this style over the async function ... syntax used for getTemperature and getCoordinates?

kelsey-steven-ada · 2025-12-10T20:11:04Z

+    const fahrenheit = (kelvin - 273.15) * (9 / 5) + 32;
+    return fahrenheit;


For flexibility and keeping to the single responsibility principle, I recommend breaking the temperature conversion out to its own function to decouple fetching the data from converting it to a specific format to display. We could still call the function from here, but if we ever need to change the format the temperature is displayed in, only code specific to that action needs to change.

This also makes things easier to test. As-is, if we wanted to unit test the code that converts the temperature, we would need to mock out the API call first. If it were it's own function, the temperature conversion code becomes very easy to independently test.

This also makes the getTemperature function easier to test – if we know that the temperature conversion code is solid and well tested but we are seeing a weird or no result displayed, then the issue must come from the network call before we try to convert the temperature.

updated，thanks

kelsey-steven-ada · 2025-12-10T20:12:35Z

+resetButton.addEventListener("click", () => {
+    cityInput.value = defaultCity;
+    cityName.textContent = defaultCity;
+});


I suggest separating registering event listeners from defining the function triggered by the event.

It is often much clearer to define the handlers in well-named functions that can then be reused and passed to the addEventListener function. That way we can register all our listeners in the same place, so it is very easy to see all of the elements which have listeners registered, and very easy to look up how they are reacting to those events by going to those well-named functions.

There are also times we may want some initial state or work done, and we want to be able to trigger that same work on a button press. For example, what if we wanted to load the real time temperature as soon as the page loaded, and also allow folks to press the button to update the temp after they change the city name? We would benefit greatly by having a single named function that did the work so we could call it when the page loaded as well as pass it to addEventListener on the relevant button.

kelsey-steven-ada · 2025-12-10T20:31:49Z

+  //console.log('currentSky = ', currentSky);
+  //console.log('skyOptions[currentSky] = ', skyOptions[currentSky]);


To keep our PRs focused on the important changes and ensure we only bring in code to the main branch that we intend to ship to users, code that is only for debugging and commented code should be removed before opening PRs.

cleaned up, thanks

kelsey-steven-ada · 2025-12-10T20:46:03Z

+const increaseTempControl = document.getElementById('increaseTempControl');
+const decreaseTempControl = document.getElementById('decreaseTempControl');


These two variables are only referenced inside of the registerTemperatureControls function to attach event listeners. Thinking about encapsulation and avoid giving code access to data it does not need, do we need to keep global variables for these elements that all functions will have access to?

updated, thanks

kelsey-steven-ada · 2025-12-22T18:50:07Z

+const init = () => {
+  registerTemperatureControls();
+  cityInput.addEventListener('input', handleCityInput);
+  currentTempButton.addEventListener('click', handleCurrentTempClick);
+  skySelect.addEventListener('change', updateSkyUI);
+  resetButton.addEventListener('click', handleResetClick);
+
+  handleCurrentTempClick();
+  handleCityInput();
+  updateTemperatureUI();
+  updateSkyUI();
+};


This works and co-locates our registration and set up, but I would strongly consider breaking this down further for best organization and keeping to the single responsibility principle. For example, we could have one function that only handles all the registration tasks, and another function that only handles setting the defaults and UI preparation, then call those two functions from here inside init() to kick everything off.

If we break up functions this way, then if we need to adjust code to set defaults, we are not also changing a function that holds code for registering event handlers. It makes it easier to test our functions and isolate issues if they should arise.

xixilele1990 and others added 14 commits December 3, 2025 10:34

coworking_agreement_signed

dcfe425

added template

ced5bad

wave 3

9ac1e74

wave 3 and 6

fff71d6

wave 3 and 6

37a333b

merge wave3,6,2

cdce499

wave 4 locationIQ

b6a9c8f

Merge branch 'Wenxin'

02eed17

wave5

02aa9d3

add Wenxin branch

2211420

2,3,4a,5,6

15444fa

wave4

a6c8d69

moved to proxy server

f0b7146

fixed sunny

59dd335

Copilot AI review requested due to automatic review settings December 4, 2025 22:30

Copilot started reviewing on behalf of Likhoram December 4, 2025 22:30 View session

Copilot finished reviewing on behalf of Likhoram December 4, 2025 22:31

Copilot AI reviewed Dec 4, 2025

View reviewed changes

Likhoram and others added 4 commits December 4, 2025 16:51

added semicolons

751f1c1

debug undefined location

5d3da31

debug undefined location

079f780

Merge branch 'main' of https://github.com/xixilele1990/weather-report

001674d

kelsey-steven-ada reviewed Dec 10, 2025

View reviewed changes

xixilele1990 added 3 commits December 16, 2025 22:43

modified code according to comments v1

4ca9e0b

global varibles to local

af80296

remove if statement

39d40ef

kelsey-steven-ada reviewed Dec 22, 2025

View reviewed changes

reorganized global variables

315b6cc

	const cityInput = document.getElementById("cityNameInput")
	const cityInput = document.getElementById("cityNameInput");

	const resetButton = document.getElementById("cityNameReset")
	const resetButton = document.getElementById("cityNameReset");


		//wave 3

		const cityName = document.getElementById("headerCityName")

		<span>For the lovely city of
		<span id="headerCityName" class="header__city-name"></span></span>


		const currentTempButton = document.getElementById('currentTempButton');

		const handleCurrentTempClick = async () => {

		const fahrenheit = (kelvin - 273.15) * (9 / 5) + 32;
		return fahrenheit;

		//console.log('currentSky = ', currentSky);
		//console.log('skyOptions[currentSky] = ', skyOptions[currentSky]);

		const increaseTempControl = document.getElementById('increaseTempControl');
		const decreaseTempControl = document.getElementById('decreaseTempControl');

Conversation

Likhoram commented Dec 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

kelsey-steven-ada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xixilele1990 Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xixilele1990 Dec 17, 2025 •

edited

Loading