Ofek/api+modal #23
Ofek/api+modal #23
Conversation
…ling for returning to countries grid
…ing and closing the menu
- Removed Hebrew comments and unused code. - Defined CSS colors as variables. - Extracted magic numbers into named variables. - Refactored theme logic and localStorage into reusable functions. - Used map for regions list and applied shorter syntax where applicable. - Applied destructuring to props for better readability.
|
|
||
| if (savedTheme) { | ||
| document.body.classList.toggle("dark-theme", savedTheme === "dark"); | ||
| return savedTheme; |
There was a problem hiding this comment.
If the user does not have any theme why not give default theme?
This way you will not have to check the value of saved theme
|
|
||
| useEffect(() => { | ||
| fetch("https://restcountries.com/v3.1/all") | ||
| .then((response) => { |
There was a problem hiding this comment.
Hardcoded strings like this should be placed inside constants folder.
Another thing - it is recommended to take all of the logic that handles something and extract it to service/ under the api folder.
|
|
||
| const handleDataError = (message) => { | ||
| console.error("Error fetching countries:", message); | ||
| setError("Failed to load countries. Please try again later."); |
| <section className="filters"> | ||
| <div className="container"> | ||
| {countries.length === 1 && ( | ||
| <button className="button-all-countries" onClick={() => setCountries(allCountries)}> |
There was a problem hiding this comment.
Save this magic number (1) into a constant variable
| @@ -1,3 +1,24 @@ | |||
| :root { | |||
| --color-black: #111517; | |||
| --color-dark-gray: #202c37; | |||
| <ul className="country-brief"> | ||
| <li><strong>Population:</strong> {population.toLocaleString()}</li> | ||
| <li><strong>Region:</strong> {region}</li> |
There was a problem hiding this comment.
It is better to style your tags via css and not with strong tag
| <p><strong>Population:</strong> {population.toLocaleString()}</p> | ||
| <p><strong>Region:</strong> {region}</p> | ||
| <p><strong>Subregion:</strong> {subregion}</p> | ||
| <p><strong>Capital:</strong> {capital ? capital[0] : "N/A"}</p> |
There was a problem hiding this comment.
Same comment in here about the strong tag
| <p><strong>Capital:</strong> {capital ? capital[0] : "N/A"}</p> | ||
| <p><strong>Area:</strong> {area.toLocaleString()} km²</p> | ||
| <p><strong>Currencies:</strong> {currencies ? Object.values(currencies).map(c => c.name).join(", ") : "N/A"}</p> | ||
| <p><strong>Languages:</strong> {languages ? Object.values(languages).join(", ") : "N/A"}</p> |
There was a problem hiding this comment.
For readability, extract the logic function into a variable and call it
| } | ||
|
|
||
| const regions = ["All", "Africa", "Americas", "Asia", "Europe", "Oceania"]; | ||
|
|
There was a problem hiding this comment.
Good practice to save those inside a variable
| <ul onClick={(e) => action(e.target.getAttribute("data-region"))}> | ||
| {regions.map((region) => ( | ||
| <li key={region} data-region={region}>{region}</li> | ||
| ))} |
There was a problem hiding this comment.
Is it possible for 2 countries to be in the same region?
If yes then think about different key for the li ( we don`t want to have 2 of the same key)
added modal and api fetch