-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
First solution #1698
base: master
Are you sure you want to change the base?
First solution #1698
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.
Great job on implementing the task requirements! 🎉 While there are a few minor issues, such as the use of HashRouter
, handling the 'no people' message, and the findParentLink
function, they don't significantly impact the functionality. Keep up the good work and consider these improvements for future projects! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -1,166 +1,24 @@ | |||
import { Loader } from './components/Loader'; | |||
|
|||
import { Routes, Route, Navigate } from 'react-router-dom'; |
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 task description specifies using HashRouter
instead of the default BrowserRouter
to ensure links can be shared on GitHub pages. Consider wrapping the Routes
component with HashRouter
.
src/components/PersonForTable.tsx
Outdated
} | ||
const parent = people.find(person => person.name === name); | ||
|
||
return parent?.slug; |
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 findParentLink
function should return null
or undefined
instead of parent?.slug
when the parent is not found. This will prevent generating incorrect links when the parent does not exist.
src/pages/PeoplePage.tsx
Outdated
{people.length === 0 && ( | ||
<p data-cy="noPeopleMessage">There are no people on the server</p> | ||
)} |
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 people.length === 0
for displaying the 'no people' message should also check if !loading
to avoid showing this message while data is still being fetched.
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 implementation meets all the task requirements and constraints effectively. The code is well-structured, and all components are functioning as expected. Keep up the excellent work, and continue to pay attention to detail as you have done here. 😊
✨ 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.
LGTM!
DEMO LINK