create button and add the style #22
Conversation
❌ Deploy Preview for rainyplaytime failed.
|
There was a problem hiding this comment.
Nice @Sandra-Duarte
Please update your branch (that means merge 'master' into your branch so you get the latest changes from the master branch) and resolve the conflicts so we can merge (you can resolve them in VS Code to visualise the issues).
src/App.js
Outdated
| <button> FIND WEATHER</button> | ||
|
|
There was a problem hiding this comment.
This button should live inside the Header component @Sandra-Duarte
| <CurrentWeather weatherInfo={weatherInfo} /> | ||
| <FutureWeather weatherInfo={weatherInfo} /> |
There was a problem hiding this comment.
Do you need to pass ALL the data to both these components? It's always best to pass down just what a component needs.
In this case: CurrentWeather only needs one item from the array of weather in the API response. And FutureWeather needs 7 items (according to the design which is our source of truth here), so you need to make a small array of 7 items here, and pass down that small array to FutureWeather instead of the whole thing
src/WeatherImages/WeatherImages.jsx
Outdated
| { | ||
| infWeather < 300 ? <img src={Storm} /> | ||
| : infWeather >= 300 && infWeather <= 499 ? <img | ||
| src={Drizzle} /> : infWeather >= 500 && infWeather <= 599 ? <img | ||
| src={Rain} /> : infWeather >= 600 && infWeather <= 699 ? <img | ||
| src={Snow} /> : infWeather >= 700 && infWeather <= 799 ? <img | ||
| src={Fog} /> : infWeather === 800 ? <img | ||
| src={Clear} /> : infWeather === 801 ? <img | ||
| src={PartlyCloudy} /> : infWeather >= 802 && infWeather <= 805 ? <img | ||
| src={MostlyCloudy} /> : | ||
| <p>No Image Found</p> | ||
| } |
There was a problem hiding this comment.
Good logic here. As a side note on best practices, when there is a complex conditional statement like this one, it's best to move it outside the component's JSX because it makes it hard to read. You could move this to a function (inside the component but before the return) like for example:
const selectIconById = (id) => {
// move your logic here and return either the icon name or
// the whole <img /> element with the correct icon in the src
}
| // const WeatherIcons = (infWeather)=>{ | ||
| // return ( | ||
| // infWeather < 300 ? <img src={Storm} /> | ||
| // : infWeather >= 300 && infWeather <= 499 ? <img | ||
| // src={Drizzle} /> : infWeather >= 500 && infWeather <= 599 ? <img | ||
| // src={Rain} /> : infWeather >= 600 && infWeather <= 699 ? <img | ||
| // src={Snow} /> : infWeather >= 700 && infWeather <= 799 ? <img | ||
| // src={Fog} /> : infWeather === 800 ? <img | ||
| // src={Clear} /> : infWeather === 801 ? <img | ||
| // src={PartlyCloudy} /> : infWeather >= 802 && infWeather <= 805 ? <img | ||
| // src={MostlyCloudy} /> : | ||
| // <p>No Image Found</p> | ||
| // )} |
There was a problem hiding this comment.
It's best not to commit commented out code like this. I would delete it.
Especially because your WeatherImage component is fulfilling this function on line 29 :)
| const CurrentWeather = ({weatherInfo}) => { | ||
|
|
||
| if(weatherInfo.length > 0){ | ||
|
|
||
| const infWeather = weatherInfo[0].weather[0].id |
There was a problem hiding this comment.
As mentioned in my comment in App.js, this component should only receive weatherInfo[0] here, so then you will be able to extract what you need from that object directly in here
| <div className="weather-container" style={{display: "flex", justifyContent:"center"}}> | ||
| { | ||
| weatherInfo.length > 0 ? | ||
| weatherInfo.slice(1, 7).map((item) => { |
There was a problem hiding this comment.
Good that you're slicing the data here 👍🏼
As I said in my comment in App.js, you could be slicing your data up there in the parent, and only passing down the sub-array to FutureWeather
| // axios request | ||
| // try { | ||
|
|
||
| // } | ||
| // catch(err){ | ||
|
|
||
| // } |
There was a problem hiding this comment.
Avoid committing comments like this, unless they are particularly useful to the other developers working on the project with you.
| appearance: none; | ||
| background-color: var(--theme-color--button); | ||
| color: var(--theme-color--ink); | ||
| padding: var(--theme-spacing--2); | ||
| text-transform: uppercase; |
There was a problem hiding this comment.
Try and make this button looks as much like the design provided as possible. Replicating designs into CSS is a key front-end developer skill.
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.