Skip to content

add task solution#3803

Open
vectormeleshko wants to merge 2 commits into
mate-academy:masterfrom
vectormeleshko:master
Open

add task solution#3803
vectormeleshko wants to merge 2 commits into
mate-academy:masterfrom
vectormeleshko:master

Conversation

@vectormeleshko

Copy link
Copy Markdown

No description provided.

@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, while your solution works functionally, it still violates several explicit checklist requirements that are considered critical for this task. In src/App.jsx and src/components/Person/Person.jsx, you partially destructure props but then repeatedly access person.* instead of fully destructuring the needed fields, which goes against [CHECKLIST ITEM #1] about using destructuring for props access. You also have a ternary expression like {person.sex === 'm' ? 'wife' : 'husband'} written on a single line, which violates [CHECKLIST ITEM #2] that each part of a ternary (condition, ? branch, : branch) must be on separate lines. Additionally, your marriage-related conditional rendering uses {person.isMarried ? (...) : (...)} directly in JSX with multiple branches, conflicting with [CHECKLIST ITEM #3] that asks you to extract a separate variable for the condition or precomputed text before returning JSX.

To fix this, at the top of Person destructure everything you need from person, for example const { name, age, sex, isMarried, partnerName } = person;, and then use those variables instead of person.* in your JSX. Refactor the ternary to a multi-line format, such as:

const partnerLabel = sex === 'm'
  ? 'wife'
  : 'husband';

so that condition and both branches are on their own lines. Finally, compute a separate variable for what you render about marriage status (for example const partnerText = isMarried ? ... : ...;) and then render {partnerText} in JSX instead of embedding the entire conditional directly. Functionally you did a good job and met the task’s behavior requirements; once you align the destructuring, ternary formatting, and conditional structure with the checklist, this will be in great shape.


✨ 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/App.jsx
@@ -1,5 +1,6 @@
import React from 'react';

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 implementation partially destructures props but then repeatedly accesses person.*. To align with checklist item #1: "Use destructuring for getting access to values of props object", consider destructuring the needed fields from person (e.g. const { name, age, sex, isMarried, partnerName } = person;) and then using them directly.

Comment thread src/App.jsx
import React from 'react';
import './App.scss';
import { Person } from './components/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.

This inline age render uses person.age && <p ...> and also relies on person.age inside JSX. While functionally correct, it still uses the nested person object instead of destructured values, which conflicts with checklist item #1 about using destructuring for props access.

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.

The marriage-related rendering mixes the condition person.isMarried directly in JSX with multiple branches, which matches the "Bad example" pattern in checklist item #3 about "Avoid putting several cases to conditional rendering". Consider extracting a separate variable (e.g. const isMarried = ...) or even a precomputed partnerText to simplify the conditional in JSX.

Comment thread src/components/Person/Person.jsx Outdated
{person.age && <p className="Person__age">I am {person.age}</p>}
{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.

This violates checklist item #2: Format ternary operator operands correctly - move each operand to the separate line. The expression {person.sex === 'm' ? 'wife' : 'husband'} is on one line and should be broken so that the condition, ? operand, and : operand are each on their own lines.

Comment thread src/components/Person/Person.jsx Outdated
<section className="Person">
<h2 className="Person__name">My name is {person.name}</h2>
{person.age && <p className="Person__age">I am {person.age}</p>}
{person.isMarried ? (

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 to conditional rendering. Create separate variable for the condition. You directly use {person.isMarried ? (...) : (...)} in JSX; instead, you should compute the condition or result in a separate variable before returning JSX.

@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

Your solution is approved because it correctly implements all required behaviors for the Person and App components. The App component uses Person exactly three times and passes the full person objects (misha, olya, alex) via the person prop, keeping those constants unchanged. In Person, you destructure person from props and then destructure its fields, satisfying [CHECKLIST ITEM #1] and avoiding any direct props.* usage. The marriage and partner text logic is correct: married men are shown with a wife, married women with a husband, and the unmarried person displays I am not married. The age is rendered only when present, so olya correctly has no age displayed, and the required class names (Person, Person__name, Person__age, Person__partner) are used in the JSX. Your handling of conditional rendering is clean and follows the guidelines: complex logic is moved into precomputed variables instead of inline multi-case JSX, which aligns with [CHECKLIST ITEM #2] and [CHECKLIST ITEM #3]. Overall, the code meets the functional and stylistic requirements of the task, and no critical or blocking issues remain; nice work bringing everything in line with the checklist and description.


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

Footnotes

  1. Rate AI review example

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