Final edition #30
Final edition #30
Conversation
This is a copy of jobs I've done before in other repository so I'm pushing all changes together here.
|
London9-Mana-Ghabraei-Frontend-RainyPlaytime |
|
|
||
| <section className="todayForecast"> | ||
| <TodayForecast | ||
| weatherId={data?.list?.[0]?.weather?.[0]?.id} |
There was a problem hiding this comment.
Maybe if the data is missing, you could show a message instead? At the moment, running locally, it's showing blank values / NaN
| <section className="todayForecast"> | ||
| <TodayForecast | ||
| weatherId={data?.list?.[0]?.weather?.[0]?.id} | ||
| description={data?.list?.[0]?.weather?.[0]?.description} |
There was a problem hiding this comment.
You could destructure these values, to make it a bit more readable, eg -
const [firstItem] = data.list[0]
| </section> | ||
| <section className="hourlyForecast"> | ||
| {data?.list?.splice(0, 7)?.map((future) => ( | ||
| <HourlyForecast |
There was a problem hiding this comment.
Every item in a list should be given a key prop (doc)
src/App.scss
Outdated
| object-fit: cover; | ||
| } | ||
|
|
||
| .description{ |
There was a problem hiding this comment.
One way to avoid duplicating these two lines is to declare a utility class eg -
.center-horizontal {
display: flex;
justify-content: center;
}
Then use that class on the 3 elements in addition to .description, .temp and .current-container
|
|
||
| return ( | ||
| <header className="header"> | ||
| <input |
There was a problem hiding this comment.
Best to wrap this in a <form /> element so the user can submit by pressing enter while in an input
| <input | ||
| className="Location" | ||
|
|
||
| placeholder="Type in a city name" |
There was a problem hiding this comment.
For accessibility, inputs should have labels. If the label isn't visible in the design, you can hide it except for screen readers with a .visually-hidden class (ref - https://css-tricks.com/inclusively-hidden/)
| }; | ||
|
|
||
| function selectImage(weatherId) { | ||
| if (weatherId < 300) { |
There was a problem hiding this comment.
Could alternatively use a switch statement here
| <div> | ||
| <Icon weatherId={weatherId} /> | ||
| <h2 className="description">{description}</h2> | ||
| <h3 className="temp"> |
There was a problem hiding this comment.
This doesn't need to be a heading as there's no content under it
| </h3> | ||
|
|
||
| <div className="current-container"> | ||
| <h4 className="humid-press">Humidity : {humidity}%</h4> |
There was a problem hiding this comment.
These can be divs instead of subheadings
This is a copy of jobs I've done before in other repository so I'm pushing all changes together here.
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.