Skip to content

ready#3761

Open
IronHedgehog wants to merge 2 commits into
mate-academy:masterfrom
IronHedgehog:develop
Open

ready#3761
IronHedgehog wants to merge 2 commits into
mate-academy:masterfrom
IronHedgehog:develop

Conversation

@IronHedgehog
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 meets all functional requirements—the Person component renders correctly, handles conditional age display, applies gender-based partner terminology, and shows the appropriate unmarried message. However, there are three explicit checklist violations in src/components/Person/Person.jsx that need to be addressed:

  1. [CHECKLIST ITEM #1] Currently destructuring only { person } from props and accessing nested properties like person.name. Change to destructure individual properties: const { name, age, sex, isMarried, partnerName } = props.person; or const { person: { name, age, sex, isMarried, partnerName } } = props;

  2. [CHECKLIST ITEM #2] The ternary operator {person.sex === 'm' ? 'wife' : 'husband'} needs formatted operands on separate lines:

{person.sex === 'm'
  ? 'wife'
  : 'husband'
}
  1. [CHECKLIST ITEM #3] Extract the partner label condition to a variable before the return statement: const partnerLabel = person.sex === 'm' ? 'wife' : 'husband';

These are code style requirements from your task checklist that enforce consistent patterns across your codebase. Please apply these fixes to align with the stated guidelines.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/components/Person/Person.jsx Outdated
@@ -1 +1,13 @@
// 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.

Checklist item #1 violation: Use destructuring for individual properties like name, age, sex, etc. Instead of destructuring only person and accessing nested properties like person.name, destructuring should be: const { name, age, sex, isMarried, partnerName } = person; or const { person: { name, age, sex, isMarried, partnerName } } = props;

Comment thread src/components/Person/Person.jsx Outdated
Comment on lines +5 to +7
{person.isMarried ? (
<p className="Person__partner">
{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 a separate line. The ternary {person.sex === 'm' ? 'wife' : 'husband'} should be formatted with ? and : on separate lines.

Comment thread src/components/Person/Person.jsx Outdated
Comment on lines +5 to +7
{person.isMarried ? (
<p className="Person__partner">
{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 condition for determining partner label (person.sex === 'm' ? 'wife' : 'husband') should be extracted to a separate variable like const partnerLabel = person.sex === 'm' ? 'wife' : 'husband';

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 meets all functional requirements. The Person component correctly renders details, handles conditional age display, applies gender-based partner terminology, and displays the unmarried message when appropriate. The class names are consistent across components. There are two checklist items worth noting for future improvement: [CHECKLIST #1]: The current approach destructures { person } from props first, then destructures person again. You could consolidate this into a single destructuring: const { person: { name, age, sex, isMarried, partnerName } } = props; [CHECKLIST #2]: The ternary operator on line 10 could have its operands on separate lines for better readability: {isMarried ? ${partnerName} is my ${partnerLabel} : 'I am not married'} However, since the code is functionally complete and tests are passing, these are minor style considerations. Great work on getting the core functionality right!


✨ 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 +1 to +2
export const Person = ({ person }) => {
const { name, age, sex, isMarried, partnerName } = 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.

This violates CHECKLIST ITEM #1: Currently destructuring { person } from props first, then destructuring person again. Change to destructure individual properties directly from props.person: const { person: { name, age, sex, isMarried, partnerName } } = props;

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