Create Main.js #19
Create Main.js #19
Conversation
❌ Deploy Preview for rainyplaytime failed.
|
|
fixes #6 |
src/Main.js
Outdated
| <div> | ||
| {/* fetch the data | ||
| images | ||
| current Weather | ||
| future Weather */} | ||
| </div> |
There was a problem hiding this comment.
2 things:
- can you use a more semantic HTML element here instead of a
div? - And how will we pass in content as a prop, inside of
<Main>? Remember we will be passing whole components into Main from inside ofApp.js
src/components/Header/Header.jsx
Outdated
| useEffect(() => { | ||
| fetch( | ||
| `https://api.openweathermap.org/data/2.5/weather?q=liverpool&units=metric&appid=3b86046cce0de3be7cfa8369f4540b37` | ||
| ) | ||
| .then((res) => res.json()) | ||
| .then((data) => { | ||
| setData(data); | ||
| }) | ||
| .catch((e) => console.log(e.message)); | ||
| }, []); |
There was a problem hiding this comment.
This is better but you are still saving the data in 2 different places (lines 9 and 18). You only need one.
I think what you need to do is:
onChangein the text input, save the user's input, just like you've done - don't change anything here it's perfect.- on search button click, trigger the API call to fetch the new data (for the new city we just saved). How will you do this? useEffect is good when you want to react to a change in value, but in this particular case, you want to react to a button click instead. I recommend not using useEffect here. How about just a simple
getNewWeatherfunction? - Replace the word 'liverpool' with
${city}so the API call changes.
If you do all this correctly it should update the temperature for each new city (it works for me 😊)
There was a problem hiding this comment.
(If you're wondering when to use useEffect, it would be perfect if you didn't have a 'search' button. For example if you just had the text input but no button, then you would want to make a new API call each time the user types a new letter. In this case, you would still save 'city' onChange but you would add city as a dependency in the useEffect array, and the API call inside the useEffect would get triggered each time the value of city changes.
This is very inefficient because you would make too many calls to the API, so in this case it's best to move the API call to be triggered by a button click instead.)
There was a problem hiding this comment.
Thank you so much Lucy! It works now. I didn't realise that fetch API works without useEffect. :) Now I learned when to use useEffect.
There was a problem hiding this comment.
@zkhing please can you:
- go to the
masterbranch and pull to get the latest changes - merge the
masterbranch into your branch. - and push
You'll then be ready to merge this branch into master so every one in the team will have access to your code.
|
@LucyMac I have done pull request to master. Can you please merge my pull request. Thanks |
@LucyMac I am sorry that making you confused. My bad and I am not familiar with git and not used to it. Sometimes I don't even know what I have done in Github. I will delete this one. |
|
No problem @zkhing these things appear complicated at first :) I'm going to delete the CYF master branch so we only have CYF main from now on. Sorry if my explanation is more confusing :D |
Contributors, PR Template
Self checklist
Changelist
Briefly explain your PR.
Context
Link to ticket with
fixes #123Questions
Ask any questions you have for your reviewer.