Skip to content

Solution#3801

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

Solution#3801
ferenssofia wants to merge 2 commits into
mate-academy:masterfrom
ferenssofia:develop

Conversation

@ferenssofia

Copy link
Copy Markdown

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because there is a violation of a mandatory code style checklist item related to ternary formatting, which counts as a critical requirement. Functionally, your solution is solid: Person is used correctly in App, the entire person object is passed, age is handled as optional, and the marriage text and class names match the specification. However, in src/components/Person/Person.jsx, the ternary operator that determines the partner relation (likely near the Person__partner paragraph) is written inline, which violates [CHECKLIST ITEM #2] on ternary formatting.

To fix this, reformat that ternary so each operand is on its own line, for example:

const partnerRelation = sex === 'f'
  ? 'husband'
  : 'wife';

This change is required because the checklist explicitly demands this formatting, and checklists are part of the core task requirements. Once you adjust this ternary formatting, your solution will fully meet both the functional and style requirements. Overall, your implementation is clear and correct, and this is a small but important fix to align with the required standards.


✨ 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
const { name, age, sex, isMarried, partnerName } = person;

// Determine partner reference based on gender
const partnerRelation = sex === 'f' ? 'husband' : 'wife';

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: Readabily is everything. Format ternary operator operands correctly - move each operand to the separate line. Currently the ternary is written inline; consider breaking it into:

const partnerRelation = sex === 'f'
  ? 'husband'
  : 'wife';

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am approving your solution because all functional requirements are implemented correctly and the checklist items that impact task completion are satisfied. Person is used three times in App with the full person object passed as a single prop, and the component correctly destructures and renders name, optional age, and partner information according to the specification. The logic for marriage status and partner wording (husband/wife vs. "I am not married") behaves as required, and the class names match the prescribed Person, Person__name, Person__age, and Person__partner.

The remaining note about the inline age !== undefined && <p ...> pattern is primarily a readability/style concern and does not break functionality or violate a core requirement in a way that blocks task completion. If you want to further improve the style, you could format that conditional rendering across multiple lines for consistency, for example:

{age !== undefined && (
  <p className="Person__age">I am {age} years old</p>
)}

Overall, your work shows good understanding of props destructuring, conditional rendering, and JSX structure—nice job.


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

Footnotes

  1. Rate AI review example

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

{/* Omit age if it is not present */}
{age !== undefined && <p className="Person__age">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.

This violates [CHECKLIST ITEM #2]: "Format ternary operator operands correctly — move each operand to a separate line." The JSX conditional age !== undefined && <p ...> is using an inline ternary-like && pattern, which matches the "BAD EXAMPLE" style of putting the condition and result on the same line. Consider splitting the condition and rendered element across multiple lines for readability, similar to how you formatted partnerRelation.

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