add solution #3554
add solution #3554
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting a few changes before this can be approved. While the overall component structure is correct, there's a functional bug and a few violations of the project's coding standards.
Specifically, in src/components/Person/Person.jsx:
- Incorrect Output: The text for an unmarried person is currently "not married" but the requirement is "I am not married".
- Complex Logic in JSX: The nested ternary operator for determining the partner status should be extracted into a new variable before the
returnstatement. This is a violation of [CHECKLIST ITEM #3]. - Code Formatting: The ternary operator itself is not formatted correctly. Each operand should be on a new line for readability, as noted in [CHECKLIST ITEM #2].
Please address these points. You're on the right track!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/components/Person/Person.jsx
Outdated
| export const Person = ({ person }) => ( | ||
| <section className="Person"> | ||
| <h2 className="Person__name">My name is {person.name}</h2> | ||
| {person.age ? <p className="Person__age">I am {person.age}</p> : ''} |
There was a problem hiding this comment.
This violates checklist item #2: 'Format ternary operator operands correctly - move each operand to the separate line'. The ternary operator should be split across multiple lines for better readability.
src/components/Person/Person.jsx
Outdated
| <section className="Person"> | ||
| <h2 className="Person__name">My name is {person.name}</h2> | ||
| {person.age ? <p className="Person__age">I am {person.age}</p> : ''} | ||
| <p className="Person__partner">{`${person.isMarried ? `${person.sex === 'm' ? `${person.partnerName} is my wife` : `${person.partnerName} is my husband`}` : 'not married'}`}</p> |
There was a problem hiding this comment.
This line has multiple issues:
- The text for an unmarried person should be "I am not married", but the current output is "not married".
- This violates checklist item Solution #2 regarding the formatting of ternary operators.
- This also violates checklist item Resolved #3: 'Avoid putting several cases to conditional rendering. Create separate variable for the condition.'
Please extract the logic for generating the partner status message into a variable before the return statement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this submission! You've successfully addressed all the previous feedback, and the solution now meets all functional requirements. The code is clean and works as expected, so I am approving your work. For future improvements, you could make the Person.jsx component even more readable by destructuring the person object itself (e.g., const { name, age } = person;). This would remove the need to repeat person. when accessing its properties in the JSX. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -1 +1,19 @@ | |||
| // export const Person = ({ person }) => (); | |||
| export const Person = ({ person }) => { | |||
There was a problem hiding this comment.
While you've correctly destructured props to get the person object, you can take it a step further to fully adhere to [CHECKLIST ITEM #1]. Consider destructuring the person object as well to get direct access to its properties (name, age, isMarried, etc.). This will make the component's body cleaner by removing the repetitive use of person..
2 participants
DEMO LINK