Skip to content

Completed the component html and showed the data based on the require…#3729

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

Completed the component html and showed the data based on the require…#3729
ValyaMeln wants to merge 1 commit into
mate-academy:masterfrom
ValyaMeln:develop

Conversation

@ValyaMeln
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 meets all functional requirements correctly — the Person component renders three times with proper props, handles optional age appropriately, and displays correct partner titles ('wife'/'husband'). The code works as expected with no broken functionality.

The review identified some code style improvements, such as extracting the nested ternary to a variable like const partnerTitle = person.sex === 'm' ? 'wife' : 'husband'; for readability, and formatting ternary operands on separate lines. These are readability enhancements rather than functional issues, and won't affect the task outcomes.

Great work on fulfilling all the core requirements — your solution is approved.


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

Footnotes

  1. Rate AI review example

Comment on lines +14 to +15
{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.

Checklist item #2 violation: Format ternary operator operands correctly - move each operand to the separate line. Current: ? 'wife' : 'husband' should be on separate lines following the GOOD EXAMPLE pattern.

Comment on lines +14 to +15
{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.

Checklist item #3 violation: Avoid putting several cases to conditional rendering. The nested ternary checking person.sex === 'm' should be extracted to a separate variable (e.g., const partnerTitle = person.sex === 'm' ? 'wife' : 'husband';) for better readability.

Comment thread src/App.jsx
import './App.scss';
import { Person } from './components/Person/Person';

export const misha = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Checklist item #1 violation: Use destructuring for getting access to values of props object. Currently accessing person.name, person.age, person.isMarried, person.partnerName, person.sex directly. Destructure these properties from person object for cleaner code.

Comment thread src/App.scss
Comment on lines 14 to 15
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This violates checklist item #3: avoid putting several cases in conditional rendering. The nested ternary person.sex === 'm' ? 'wife' : 'husband' is hard to read inside the ternary. Extract this into a separate variable before the JSX for better readability.

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