-
Notifications
You must be signed in to change notification settings - Fork 38
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
practice solution #23
base: main
Are you sure you want to change the base?
Conversation
YePolishchuk
commented
Oct 13, 2023
•
edited
Loading
edited
- DEMO LINK
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.
- Select All to see all the products.
It is impossible to select All if some user is selected
- Clear the value after the x button click.
The button doesn't clear the value
- Show a No results message if there are no products matching the current criteria
Reset All Filters button should clear all the filters.
No products
message doesn't match the design
Reset all filters
button doesn't reset the search query
})); | ||
|
||
export const App = () => { | ||
const [currentPeople, setcurrentPeople] = useState(''); |
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.
Why is it a string? The name or value is confusing
const productsCategories = productsFromServer.map(product => ({ | ||
...product, | ||
category: categoriesFromServer | ||
.find(category => category.id === product.categoryId), // find by product.categoryId // find by category.ownerId | ||
})); | ||
|
||
const products = productsCategories.map(product => ({ | ||
...product, | ||
user: usersFromServer | ||
.find(person => person.id === product.category.ownerId), // find by product.categoryId // find by category.ownerId |
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.
Can't you use only one map to add both properties?
selectedProducts = selectedProducts | ||
.filter(product => ( | ||
product.category.title === currentCategory | ||
)); |
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.
You can put all these checks into one filter method. Also, would be great to create a function for filtering and call it if query or some other filter changes
onClick={() => { | ||
setcurrentPeople(''); | ||
setQuery(''); | ||
setCurrentCategory(''); | ||
}} |
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.
Would be great to create a separate function to handle reset
<span className="is-flex is-flex-wrap-nowrap"> | ||
ID | ||
|
||
<a href="#/"> | ||
<span className="icon"> | ||
<i data-cy="SortIcon" className="fas fa-sort" /> | ||
</span> |
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.
Do you really need to hardcode all categories? You can map through them
{/* <tr data-cy="Product"> | ||
<td className="has-text-weight-bold" data-cy="ProductId"> | ||
2 | ||
</td> | ||
|
||
<td data-cy="ProductName">Bread</td> | ||
<td data-cy="ProductCategory">🍞 - Grocery</td> | ||
|
||
<td | ||
data-cy="ProductUser" | ||
className="has-text-danger" | ||
> | ||
Anna | ||
</td> | ||
</tr> | ||
|
||
<tr data-cy="Product"> |
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.
Don't forget to remove the comments