-
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
Develop #1178
base: master
Are you sure you want to change the base?
Develop #1178
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! Though, I've listed some moments that should be noted
<p className="panel-heading">Filters</p> | ||
|
||
<p className="panel-tabs" data-cy="SexFilter"> | ||
{Object.values(PersonSex).map(sexFilter => { |
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.
That's not a big deal, but you could create an enum Sex { All = '', Female = 'f', Male = 'm' } and use Object.keys() to create the same tabs from the example. A key would play a role of tab's title (All. Female. Male) and key's value would be a search parameter ('', 'f', 'm').
<div className="level is-flex-grow-1 is-mobile" data-cy="CenturyFilter"> | ||
<div className="level-left"> | ||
{CENTURIES.map(century => { | ||
const currentCenturies = centuries.includes(century) |
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.
There is too much code here. I believe it would make sense to move some part of it to a separate function. To be more specific, the part where you filter your centuries array.
|
||
<div className="column"> | ||
<div className="box table-container"> | ||
{isLoaderVisible && <Loader />} |
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.
So, I noticed one small issue here. The message 'There are no people matching the current search criteria' appears with the loader. The thing is, that you haven't received any data from the server yet, 'cause the loading is still ongoing.
So, whenever you show the loader, hide everything else. Display this error message only if the timeout was reached and the server hasn't returned anything.
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.
Your code is well-structured and meets all the task requirements effectively! 🎉 The implementation of filtering and sorting functionalities is spot on, and you've ensured that the search parameters are retained during navigation. Great job on maintaining clean and organized code. Keep up the excellent work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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! 🎉 Your code meets all the task requirements and constraints, and everything is implemented correctly. The use of URL search params, navigation, filtering, and sorting is well-executed. Keep up the excellent work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
DEMO LINK