Weather project - Solhee Jin, Renata Murzina by RenaSpb · Pull Request #7 · Ada-C23/weather-report

RenaSpb · 2025-06-05T18:11:25Z

No description provided.

…e unit as displayed temp

ellenjin · 2025-06-06T16:50:38Z

package.json

I believe that we aren't supposed to have the package-lock.json and package.json files, but they got added at some point while we were applying EsLint/Prettier -- please let us know if you need them to be deleted for grading purposes or such!

mikellewade

Renata and Solhee, great work on your Weather Report project!

mikellewade · 2025-06-15T20:14:44Z

index.html

+      </section>
+    </div>
+
+    <script src="https://unpkg.com/axios/dist/axios.min.js"></script>


mikellewade · 2025-06-15T20:16:16Z

index.html

@@ -1,15 +1,87 @@
-<!DOCTYPE html>
+<!doctype html>


Is there a reason this got changed to lowercase, or was it a side effect of something else?

Suggested change

<!doctype html>

<!DOCTYPE html>

mikellewade · 2025-06-15T20:19:12Z

index.html

+    <meta charset="UTF-8" />
+    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
+    <meta name="viewport" content="width=device-width, initial-scale=1.0" />


You are good here, but just a warning about trailing / in HTML:

Trailing slashes in void-element start tags do mark the start tags as self-closing

A word about trailing slashes preceded by unquoted attribute values

mikellewade · 2025-06-15T20:22:19Z

index.html

    <title>Weather Report</title>
-    <link rel="preconnect" href="https://fonts.gstatic.com">
-    <link href="https://fonts.googleapis.com/css2?family=Rubik&display=swap" rel="stylesheet">
+    <link rel="icon" type="image/png" href="https://www.principalrelocation.com/wp-content/uploads/2018/06/cloud.png`" />


Seems like your tab icon wasn't loading because of a (`) being left in the href.

Suggested change

<link rel="icon" type="image/png" href="https://www.principalrelocation.com/wp-content/uploads/2018/06/cloud.png`" />

<link rel="icon" type="image/png" href="https://www.principalrelocation.com/wp-content/uploads/2018/06/cloud.png" />

mikellewade · 2025-06-15T20:45:17Z

index.html

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


@RenaSpb and @ellenjin, I'm commenting here but it could be applied to your class naming in general. Your naming conventions for them are, in some cases, inconsistent (city-name__section vs city-input-wrapper) and a bit redundant (header__header). You'll want to make sure that you being consistent and meaningful when naming classes. This isn't blocking, just a suggestion to improve long-term maintainability and consistency.

Actually, you can disregard this! I see that it is from the template! LOL

mikellewade · 2025-06-15T21:50:59Z

src/index.js

+    })
+    .catch((error) => {
+      console.log('Error:', error);
+      alert('Enter right city name!');


Being that this could go wrong for a number of reasons, I 'd probably not state in the alert to enter a correct city name. That would misleading in cases where it isn't user error.

mikellewade · 2025-06-15T21:52:41Z

src/index.js

+
+const getRealWeather = (lat, lon) => {
+  axios
+    .get('http://localhost:5000/weather', {


You could use a constant variable, say BASE_URL, that way you won't have to change the url in multiple places come deployment.

mikellewade · 2025-06-15T21:56:41Z

src/index.js

+    });
+};
+
+updateTempEmoji();


This could also be moved inside of the DOMContentLoaded event. But despite that, it feels a bit misplaced, this function being called in the middle of function defintions. Maybe we could move it to the top or bottom?

mikellewade · 2025-06-15T21:56:56Z

src/index.js

+  switch (selected) {
+    case 'Sunny':
+      skyDisplay.textContent = '☀️';
+      document.body.classList.add('sunny-bg');
+      break;
+    case 'Cloudy':
+      skyDisplay.textContent = '☁️';
+      document.body.classList.add('cloudy-bg');
+      break;
+    case 'Rainy':
+      skyDisplay.textContent = '🌧️';
+      document.body.classList.add('rainy-bg');
+      break;
+    case 'Snowy':
+      skyDisplay.textContent = '❄️';
+      document.body.classList.add('snowy-bg');
+      break;
+    default:
+      skyDisplay.textContent = '';
+  }


mikellewade · 2025-06-15T22:00:10Z

src/index.js

+let isFahrenheit = true;
+let currentTemp = 50;
+
+function updateCityName() {


In some places you use the function keyword to declare functions and others you use const fnName = ().... You should make sure that you are being consistent in their uses for maintainability and predicatabliity (based on declare them one could have hoisting and the other won't). @ellenjin @RenaSpb

ellenjin and others added 30 commits June 2, 2025 10:27

Added content to index.css

47ea50c

added axios

3e2cda5

Added in template in index.html

c5723b6

Implemented Wave 3

9020854

wave2

171089b

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

1ee8298

wave 2 updated

2ed6f79

Started working on Wave 4

d87c105

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

7ad75dd

wave 4

0aee096

css fixed

b3e23b2

css updated

945835e

adding css styles

37ddc14

Merging

260343e

Merge, complete up to wave 4

c1b17c4

wave 6

10a40f1

narrow for input added

432e99e

Applied Prettier to project, adjusted for conflicts with esLint

dcff749

2 spaces applied

7b7c413

merging

919c5fa

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

6ea3043

Fixed .gitignore

2d8f9ab

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

5c9525d

js file formated

52b41a8

F/C button

9697672

Completed Wave 5 -- sky dropdown

d8ebf68

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

6dbed68

Completed Wave 5, adjusted displayed celcius/fahrenheit to be the sam…

37ff6db

…e unit as displayed temp

index.html updated

4e7036a

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

fc10098

RenaSpb added 2 commits June 5, 2025 09:50

background

615419a

weather change

8946193

ellenjin reviewed Jun 6, 2025

View reviewed changes

mikellewade reviewed Jun 15, 2025

View reviewed changes

	<link rel="icon" type="image/png" href="https://www.principalrelocation.com/wp-content/uploads/2018/06/cloud.png`" />
	<link rel="icon" type="image/png" href="https://www.principalrelocation.com/wp-content/uploads/2018/06/cloud.png" />

Conversation

RenaSpb commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikellewade 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!

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!

3 participants