Skip to content

attempt 1#3634

Open
prokopenkooleh wants to merge 3 commits intomate-academy:masterfrom
prokopenkooleh:develop
Open

attempt 1#3634
prokopenkooleh wants to merge 3 commits intomate-academy:masterfrom
prokopenkooleh:develop

Conversation

@prokopenkooleh
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

Great job on the overall structure and implementation! Your components are well-organized, and you've correctly handled the logic for the partner's status. However, some changes are needed before this can be approved. The main issue is in src/components/Person/Person.jsx, where the optional age property is not handled correctly. The current check age === false doesn't work when the property is missing (its value is undefined), which results in rendering "I am undefined". A better approach would be to conditionally render the age paragraph only if the age property exists. Additionally, please format the ternary operator used for partnerText across multiple lines to improve readability, as this violates a checklist requirement [CHECKLIST ITEM #2]. Once these points are addressed, your solution will be ready!


✨ 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

<section className="Person">
<h2 className="Person__name">My name is {name}</h2>

<p className="Person__age">{age === false ? '' : `I am ${age}`}</p>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are two issues on this line:

  1. The logic to check for the age property is incorrect. The requirement is to omit the age if it's not present in the object. When a property is not present, its value is undefined. The current check age === false will not work correctly and will result in rendering "I am undefined".

  2. This one-line ternary operator violates checklist item Solution #2, which requires formatting it across multiple lines for better readability.

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

Good progress on this task, but a couple of issues are preventing approval.

First, a unit test is failing because the age element isn't being correctly omitted. When age is not provided, your code renders an empty <p class="Person__age"></p>, but the requirement is to remove the element from the DOM entirely. You can fix this by wrapping the entire <p> tag in your conditional check, not just its content.

// Correct approach
{age && (
  <p className="Person__age">
    I am {age}
  </p>
)}

Second, the code style issue with the ternary operator mentioned in the last review still needs to be addressed. Please format it across multiple lines for readability, as required by [CHECKLIST ITEM #2].

Once these two points are fixed, the solution will be ready!


✨ 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

let partnerText = '';

if (isMarried) {
partnerText = sex === 'm' ? ` is my wife` : ` is my 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.

This violates checklist item #2: 'Format ternary operator operands correctly - move each operand to the separate line'. Please split this ternary operator onto multiple lines for better readability.

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

Great job on this task, and thank you for addressing the previous feedback! The component now correctly handles conditional rendering and formatting.

I'm approving this solution so you can move forward, but I have one suggestion for improvement in Person.jsx. While your logic for displaying the partner status works, it could be more robust. A cleaner approach is to create a single variable that holds the entire message. This consolidates the logic and makes your component's rendering simpler.

For example:

const partnerStatus = isMarried
  ? `${partnerName} is my ${sex === 'm' ? 'wife' : 'husband'}`
  : 'I am not married';

// Then in your JSX:
<p className="Person__partner">{partnerStatus}</p>

This is a minor point, and your solution is functionally correct. Keep up the excellent work!


✨ 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 +20 to +23
<p className="Person__partner">
{partnerName}
{partnerText}
</p>
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 logic works, but it could be more robust. It relies on partnerName being undefined when a person is not married. A cleaner way to handle this is to create a single variable that holds the complete partner status message based on the isMarried flag. This would make your component's rendering logic simpler and more explicit.

For example:

const partnerStatus = isMarried
  ? `${partnerName} is my ${sex === 'm' ? 'wife' : 'husband'}`
  : 'I am not married';

// later in JSX
<p className="Person__partner">{partnerStatus}</p>

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