Skip to content

add task#3742

Open
ivanov-serg wants to merge 1 commit into
mate-academy:masterfrom
ivanov-serg:develop
Open

add task#3742
ivanov-serg wants to merge 1 commit into
mate-academy:masterfrom
ivanov-serg:develop

Conversation

@ivanov-serg
Copy link
Copy Markdown

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your implementation successfully fulfills all functional requirements from the task description. The Person component correctly renders all person details, handles optional age display, applies the correct partner terminology based on sex, and shows the 'I am not married' message when appropriate. The three persons are rendered in App.jsx with the complete person object passed as a prop. While there are some code style violations regarding destructuring depth and ternary formatting, these are formatting preferences rather than functionality issues. The component works correctly for all specified inputs and produces the expected output.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

@@ -1 +1,15 @@
// export const Person = ({ person }) => ();
export const Person = ({ person }) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Violates checklist item #1: Use destructuring for getting access to values of props object. Currently { person } destructures only the outer prop, but individual properties like name, age, sex, isMarried, partnerName are accessed via person.propertyName. Destructure all person properties directly: ({ name, age, sex, isMarried, partnerName }).

Comment on lines +8 to +10
{person.isMarried
? `${person.partnerName} is my ${person.sex === 'm' ? 'wife' : 'husband'}`
: 'I am not married'}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Violates checklist item #2: Format ternary operator operands correctly - move each operand to the separate line. The ternary {person.isMarried ? ... : ...} has operands on the same line. Format as:

{person.isMarried
  ? `...`
  : 'I am not married'}

{person.age && <p className="Person__age">I am {person.age}</p>}
<p className="Person__partner">
{person.isMarried
? `${person.partnerName} is my ${person.sex === 'm' ? 'wife' : 'husband'}`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Violates checklist item #2: The nested ternary for determining partner relation (person.sex === 'm' ? 'wife' : 'husband') should also have operands on separate lines for readability.

// export const Person = ({ person }) => ();
export const Person = ({ person }) => {
return (
<div className="person">
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer div uses className="person" (lowercase) but according to the requirements, class names should match Person, Person__name, Person__age, Person__partner. Consider using className="Person" to be consistent with BEM naming convention used in other classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants