Countries-React-Ofek #7
Conversation
…ling for returning to countries grid
…ing and closing the menu
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Generally do not comment out unused code- remove it
Do not add comments that explain variable and function, the name should explain what they do.
Do not ad Hebrew comments ( I saw that you did it a lot inside your css file)
Save your css colors inside variables
Other than that I left you comments regarding other stuff as well
| "no-unused-vars": [ | ||
| "warn", | ||
| { "varsIgnorePattern": "^_" } // מתעלם ממשתנים שמתחילים ב-_. | ||
| ], |
There was a problem hiding this comment.
Please don`t use comments in hebrew
| function App() { | ||
| const [theme, setTheme] = useState(() => { | ||
| const savedTheme = localStorage.getItem("theme"); | ||
|
|
There was a problem hiding this comment.
You can extract all logic related to the localStorage into storageService that will expose methods to use, for example :
storageSession.getIemFromLocalStora(id)etc...
|
|
||
| if (savedTheme) { | ||
| document.body.classList.toggle("dark-theme", savedTheme === "dark"); | ||
| return savedTheme; |
There was a problem hiding this comment.
Good usage of the second argument of toggle function
| * Resets the countries list to show all available countries. | ||
| */ | ||
| const showAllCountries = () => { | ||
| setCountries(countriesData); |
There was a problem hiding this comment.
It is not a good practice too add comment that explain what a function of a variable is doing, the code should explain itself
| <section className="filters"> | ||
| <div className="container"> | ||
| {countries.length === 1 && ( | ||
| <button className="button-all-countries" onClick={showAllCountries}> |
There was a problem hiding this comment.
Do not use magic numbers, extract them into variable with meaningful name.
For some one else that does not know your context of what you are working on, they don't know what 1 represents
| */ | ||
| const Country = ({country}) => { | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Destruct your props :
const Country = ({ country: { flag, name, population, region, capital } }) => {
return (
<>
<div className="country-flag">|
|
||
| return ( | ||
| <div className={`dropdown-wrapper ${isOpen ? "open" : ""}`}> | ||
| <div className="dropdown-header flex flex-jc-sb flex-ai-c" onClick={toggleDropdown}> |
There was a problem hiding this comment.
Here is a shorter syntax :
<div className={`dropdown-wrapper ${isOpen && "open"}`}>
src/components/RegionFilter.jsx
Outdated
| <div className="dropdown-body"> | ||
| <ul onClick={action}> | ||
| <li data-region="All">All</li> | ||
| <li data-region="Africa">Africa</li> |
There was a problem hiding this comment.
use map instead :
const regions = ["All", "Africa", "Americas", "Asia", "Europe", "Oceania"];
<ul onClick={action}>
{regions.map((region) => (
<li key={region} data-region={region}>{region}</li>
))}
</ul>| onClick={toggleTheme}> | ||
| <i className={`fa-regular ${theme === "dark" ? "fa-sun" : "fa-moon"} theme-icon`}></i> | ||
| <span className="theme-text">{theme === "dark" ? "Light Mode" : "Dark Mode"}</span> | ||
| </button> |
There was a problem hiding this comment.
Extract theme === "dark" into a variable to avoid code duplication
- 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.
2 participants
No description provided.