-
Notifications
You must be signed in to change notification settings - Fork 22
or yossef #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
or yossef #24
Conversation
Tamir198
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work overall.
Some general notes :
When you have hardcoded strings extract them into constants file
|
|
||
| const Country = () => { | ||
| const Country = ({ name, flag, population, region, capital, onClick }) => { | ||
| return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on destruct the props, by doing this you are flattening your data and sending the most simple props to your componenet
| </button> | ||
| <h2>{country.name}</h2> | ||
| <img src={country.flag} alt={`Flag of ${country.name}`} /> | ||
| <p><strong>Population:</strong> {country.population.toLocaleString()}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In here you better destruct your props as well
| <p><strong>Population:</strong> {country.population.toLocaleString()}</p> | ||
| <p><strong>Region:</strong> {country.region}</p> | ||
| <p><strong>Capital:</strong> {country.capital}</p> | ||
| <p><strong>Subregion:</strong> {country.additionalInfo.subregion}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing, do not use the strong tag, use css instead
| const fetchCountries = async () => { | ||
| try { | ||
| const response = await fetch("https://restcountries.com/v3.1/all"); | ||
| const data = await response.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save the hardcoded string into constants, and the fetch function should be extracted to a service that handles api requests
done API