-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add task solution #1171
base: master
Are you sure you want to change the base?
add task solution #1171
Conversation
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 Job! Have a Great New Year!
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.
Great job on your implementation! 🎉 Your code meets the task requirements and constraints effectively. The structure and functionality are well thought out, and you've done an excellent job with the routing, filtering, and sorting features. There are a few minor suggestions for improvement, such as ensuring the sorting logic is more specific and considering implementing any necessary handlers that are currently commented out. Keep up the great work, and happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
// handleSexChange: (sex: SexType) => void; | ||
// toggleCenturies: (ch: string) => void; |
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.
The handlers handleSexChange
and toggleCenturies
are commented out. If these are necessary for the component's functionality, consider implementing and using them.
.then(setPeople) | ||
.catch(() => { |
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.
Consider handling the error more specifically by logging it or displaying a more detailed message to the user.
|
||
const handleSortClick = (sortField: string) => { | ||
if (sort === sortField) { | ||
if (order) { |
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.
The condition if (order)
should be more specific, such as checking if order === 'asc'
, to ensure the sorting logic is applied correctly.
DEMO LINK