Connect to Weather API by zkhing · Pull Request #28 · CodeYourFuture/Project-RainyPlaytime

zkhing · 2023-02-21T22:25:06Z

Contributors, PR Template

Self checklist

I have committed my files one by one, on purpose, and for a reason
I have titled my PR with ISSUE NUMBER | ISSUE TITLE
I have linked my PR to the paired issue with #123
I have tested my changes
My changes follow the style guide
My changes meet the acceptance criteria

Changelist

Briefly explain your PR.

Context

Link to ticket with fixes #123

Questions

Ask any questions you have for your reviewer.

LucyMac · 2023-02-25T11:53:35Z

+        {/* <div>
          <img
            className="weather-img"
            src="https://cdn2.iconfinder.com/data/icons/weather-flat-14/64/weather02-512.png"
            alt="Current Weather"
          />
-        </div>
+        </div> */}


You can delete this

Hi Lucy, sorry for late reply! yes, I have deleted it.

LucyMac · 2023-02-25T12:00:17Z

+  } else if (weatherId === 801) {
+    return images.partlyCloudy;
+  }
+  return images.mostlyCloudy;


Nice code here @zkhing 🙌🏼

mostlycloudy should really be shown between 802 and 805. There is also an icon called unknown.svg so you could show this as your final 'else' condition.

Please make this small change, then I will approve this PR, and we can merge it in.

Thank you. Actually, my tech buddy kindly helped me with that part. :) And I added unknown.svg image now.

LucyMac · 2023-02-25T12:20:20Z

+        {/* <div>
+          <img
+            className="weather-img"
+            src="https://cdn2.iconfinder.com/data/icons/weather-flat-14/64/weather02-512.png"
+            alt="Current Weather"
+          />
+        </div> */}
+
+
+        {/* {FakeWeatherData.list.map((data) => (
+          <div key={data.dt}>Temp : {data && data.main.temp}</div>
+        ))} */}
+
+        {/* <div>{location}</div> */}
+        {/* <div>{temp && temp.list[0].main.temp}</div> */}


This is the main wrapper of all your other components. It should look something like this:

<Header /> <main> <CurrentWeather /> <FutureWeather /> </main>

You currently have all the weather icon and weather data being displayed inside your Header component. This needs to be fetched in the Header component (as you have done), but it needs to be displayed inside of <main>, in a specific component (something like CurrentWeather).

Let's get this merged in though and I'll create a ticket to move this logic to the right place in a different PR @zkhing

I have improved some changes Lucy but I am stuck and it is still incomplete yet. Please have a look at the changes that I've made and suggest me how to improve when you have time. I do not know how to slice data from API for current and future weather. Many thanks

…eature/main

LucyMac · 2023-03-02T11:11:12Z

+          {weatherData?.list?.map((current) => (
+            <CurrentWeather
+              description={current.weather.description}
+              temp={current.main.temp.toFixed()}
+              humidity={current.main.humidity}
+              pressure={current.main.pressure}
+            />
+          ))}


Let's think about this @zkhing
<CurrentWeather /> is just one item of weather. The weather at a specific time.
Unlike FutureWeather which is a list of several items of weather.

So does it makes sense to .map() over the data here? When all we want is one item?

Hi Lucy, it makes sense not to use .map( ) but I couldn't fetch current weather from API for somehow.

LucyMac · 2023-03-02T11:15:31Z

+          {weatherData?.list?.map((future) => (
+            <FutureWeather
+              time={future.dt_txt}
+              iconId={future.weather[0].id}
+              temp={future.main.temp.toFixed()}
+            />
+          ))}


Here we do want more than one item, however:

We don't want all the data available. There are 40 items in the API data, but we only want to display 7 items (we know this because we are following the design. Always use the design to decide what your code needs to do).

It would be best to pass the future weather data to the FutureWeather component, and the component itself should .map() over it and display each item. Rather than mapping here inside App.js.

I could mange to display 7 items. But positioning is incorrect and I am not sure whether I should use CSS grid instead of flexbox.

I tried to pass the future weather data to the FutureWeather component as you suggested but again I couldn't fetch data from API in child component.

You can use grid but you should be able to achieve the layout with just flexbox. It's up to you. The flex container should be full width but have some padding on the sides, so it won't actually look completely full width which is what we want.
The flex items should be placed in a row, centered and with space-between.
Then within each item, you should use flexbox again, this time to place the time, icon and temperature in a column. Again centered.

You do not need to call the API again! The whole point is to call it once in the parent, and then pass the array of 7 weather items to the FutureWeather component as a prop.
So you should add a prop called for example 'weatherItems' to the FutureWeather component, with your array of 7 items as the value.
Then inside FutureWeather you will receive props.weatherItems and .map() over it (since it is an array). And for each item in the map you can display your weather item with time, icon and temp.

Hope this makes sense! Give it a go and I'll have a look when you're ready.

…eature/main

LucyMac

Making good progress @zaw, a few more comments below.

Also, have you deployed this somewhere? It would be useful to have a link in your PR to see the deployed version of the app

LucyMac · 2023-03-08T14:18:03Z

+      <div className="description">
+        <h2>{description}</h2>
+      </div>
+
+      <div className="temp">
+        <h3>
+          Temperature : {temp_min}° to {temp_max}°C
+        </h3>
+      </div>
+
+      <div className="box">
+        <div className="sub-box">
+          <h4>Humidity : {humidity}%</h4>
+        </div>
+
+        <div className="sub-box">
+          <h4>Pressure : {pressure}</h4>
+        </div>
+      </div>


Do you need to wrap each heading inside a div? What is the intention here? Could you not put the CSS class (description, temp, sub-box etc) on the heading element itself?
Remember we only want the minimum amount of markup to make a cohesive, semantic structure for our content.

I see. I deleted some divs :) thank you

LucyMac · 2023-03-08T14:18:41Z

+  justify-content: center;
+  align-items: center;
+  width: 100%;
+  border: 1px solid #fb8500;


If you added this border to help you in your development that's fine, but don't commit it 😉

Sure.It's deleted now.

LucyMac · 2023-03-08T14:20:10Z

-      <Icon name="clear"/>
-
+        <section>
+          <WeatherIcon weatherId={weatherData?.list?.[0]?.weather?.[0]?.id} />


Why is this WeatherIcon component not part of CurrentWeather? It displays information about the current weather so it should appear in the same component that displays the temp, pressure etc.

Move it down one level inside the CurrentWeather component

I moved it down inside the CurrentWeather component.
I ended with the design like that. I know it should not be like that and this is not how I wanted. I couldn't do the exact design due to my poor CSS skill.

Thank you so much Lucy all along the way for teaching and guiding me!! I have learned a lot from you. I started final project on last Saturday therefore, I will focus on final project now. :)

Good luck with it all @zkhing 😊

zkhing · 2023-03-08T16:50:36Z

Making good progress @zaw, a few more comments below.

Also, have you deployed this somewhere? It would be useful to have a link in your PR to see the deployed version of the app

Hi Lucy, I haven't deployed anywhere. This is the branch and I don't know how to make deployed version in this PR. There is a deployed site in main but not updated version.

zkhing added 6 commits February 4, 2023 15:30

Adding weather image

08fa292

Fetching API

dac08f8

Button onclick

87d8e92

Setting temp

a18dd57

Fetch temp from API

be36012

Matching up the weather icon images

85d523f

LucyMac changed the title ~~Main~~ Connect to Weather API Feb 25, 2023

Merge branch 'main' into main

ebb724f

LucyMac reviewed Feb 25, 2023

View reviewed changes

LucyMac mentioned this pull request Feb 25, 2023

Look up / Search component #12

Open

4 tasks

zkhing added 2 commits March 1, 2023 21:16

Future weather and refactor code

accfe59

Merge branch 'main' of https://github.com/zkhing/RainyPlaytime into f…

544ee29

…eature/main

zkhing mentioned this pull request Mar 1, 2023

Create Main.js #19

Closed

6 tasks

zkhing and others added 2 commits March 1, 2023 22:07

Save to latest changes

2cbd25e

Merge branch 'main' into main

8b2d947

LucyMac reviewed Mar 2, 2023

View reviewed changes

zkhing added 3 commits March 2, 2023 23:42

Try to fetch current weather

fa398df

Merge branch 'main' of https://github.com/zkhing/RainyPlaytime into f…

dc4e844

…eature/main

Current weather working

f47a206

LucyMac reviewed Mar 8, 2023

View reviewed changes

zkhing added 2 commits March 8, 2023 14:40

Delete FutureWeather in App.js

925f7de

Move weather icon inside CurrentWeather

7c3b35c

zkhing added 4 commits June 27, 2023 11:31

Make future weather items in column and center

050e635

Make image-icons bigger

4a6f098

Make future-icons center

b458d4b

Indent code and complete the project

42f58a9

Conversation

zkhing commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributors, PR Template

Changelist

Context

Questions

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!

LucyMac Mar 3, 2023 • 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!

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

zkhing commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkhing commented Feb 21, 2023 •

edited

Loading

LucyMac Mar 3, 2023 •

edited

Loading