Skip to content

Develop#3782

Open
yarovskiy-a wants to merge 4 commits into
mate-academy:masterfrom
yarovskiy-a:develop
Open

Develop#3782
yarovskiy-a wants to merge 4 commits into
mate-academy:masterfrom
yarovskiy-a:develop

Conversation

@yarovskiy-a

Copy link
Copy Markdown

Implemented a Person component that will display person data using a given markup, and use it 3 times inside App instead of static markup.

@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 are still explicit checklist requirements not being followed, even though the core functionality works correctly.

Critical issues that must be fixed:

  1. In src/components/Person/Person.jsx you only destructure person from props, but the checklist requires destructuring the fields as well [CHECKLIST ITEM #1]. For example, instead of repeatedly using person.name, person.age, etc., destructure them at the top of the component: const { name, age, sex, isMarried, partnerName } = person; and then use these variables directly.
  2. In Person.jsx the ternary that decides between wife and husband is written inline on a single line, which violates the formatting rule [CHECKLIST ITEM #2]. You need to place the condition, the ? branch, and the : branch on separate lines for better readability.
  3. In Person.jsx (and noted in App.jsx), you embed multiple marriage conditions directly in JSX, such as {person.isMarried && (...)} and {!person.isMarried && (...), which contradicts the requirement to avoid several cases in a single conditional rendering expression [CHECKLIST ITEM #3]. Compute a variable before the return (for example, const partnerText = ... or const isMarriedText = ...) and then use that variable in the JSX.

Once you address these checklist items, your solution will fully match both the functional and stylistic requirements, and the overall structure you already have is a solid base to build on.


✨ 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
import { Person } from './components/Person/Person';

export const misha = {
name: '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.

This line contributes to violating checklist item #3: "Avoid putting several cases to conditional rendering. Create separate variable for the condition." You are combining the person.isMarried check directly with JSX; consider computing a isMarried (or similar) variable before the return and using it in the JSX instead of combining logic and rendering in one expression.

@@ -1 +1,14 @@
// 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.

This violates checklist item #1: "Use destructuring for getting access to values of props object". You destructure person from props, but then repeatedly use person.xxx instead of destructuring name, age, sex, isMarried, and partnerName from person and using them directly.

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 ternary 'wife' : 'husband' is written inline; consider breaking it into multiple lines as shown in the good example.

@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 some explicit checklist requirements are still not followed, even though the core functionality works correctly. In src/components/Person/Person.jsx, you currently destructure only person from props but then use person.name, person.age, etc.; to satisfy [CHECKLIST ITEM #1], you need to destructure the fields from person itself (e.g. const { name, age, sex, isMarried, partnerName } = person;) and then use name, age, sex, isMarried, partnerName directly in JSX. Also in Person.jsx, the ternary that chooses 'wife' or 'husband' is written on a single line; per [CHECKLIST ITEM #2], reformat it so the condition, ? branch, and : branch each go on their own line. Finally, the married/not-married rendering (!person.isMarried ? (...) : (...) and the inline {person.age && ...}) is done directly in JSX; to comply with [CHECKLIST ITEM #3], compute separate variables before the return (for example const ageElement = age ? ... : null; and const partnerContent = isMarried ? ... : ...;) and then render those variables. Functionally you’re very close and App.jsx usage looks good; once you adjust Person.jsx to follow these style/checklist rules, your solution 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 still violates checklist item #1: Use destructuring for getting access to values of props object. You destructure only person from props, but then repeatedly use person.name, person.age, etc. The checklist requires destructuring the values you use (e.g., const { name, age, sex, isMarried, partnerName } = person;) and then using those variables directly in JSX.

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.

This line uses person.age directly in JSX instead of a destructured variable. This is part of the same checklist item #1 violation: you should destructure age (and other fields) from person before the return and use age here.

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

export const misha = {
name: '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 married/not-married conditional JSX (!person.isMarried ? ... : ...) directly combines several cases in the render, which conflicts with checklist item #3: Avoid putting several cases to conditional rendering. Create separate variable for the condition.. Compute a variable (e.g., partnerContent or partnerText) before the return and render that variable here instead of the full ternary.

@@ -1 +1,15 @@
// 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.

This still violates CHECKLIST ITEM #1: "Use destructuring for getting access to values of props object". You destructure person from props, but then repeatedly access person.name, person.age, etc. Instead, destructure the fields from person itself (e.g. const { name, age, sex, isMarried, partnerName } = person;) and then use those variables in JSX.

Comment thread src/components/Person/Person.jsx Outdated
return (
<section className="Person">
<h2 className="Person__name">My name is {person.name}</h2>
{person.age && <p className="Person__age">I am {person.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 inline ternary {person.age && <p ...>} combines the age check and rendering in one expression. Given CHECKLIST ITEM #3 about avoiding several cases in conditional rendering, it would be more consistent to compute a variable (e.g. const ageElement = age ? <p>...</p> : null;) before the return and then render that variable here.

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 ternary !person.isMarried ? (...) : (...) in JSX violates CHECKLIST ITEM #3: "Avoid putting several cases to conditional rendering. Create separate variable for the condition.". Compute the married/not-married content in a variable before the return and then use that variable here instead of inlining the whole ternary.

Comment thread src/components/Person/Person.jsx Outdated
<p className="Person__partner">I am not married</p>
) : (
<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 inner ternary person.sex === 'm' ? 'wife' : 'husband' violates CHECKLIST ITEM #2: "Format ternary operator operands correctly - move each operand to the separate line". Break it into multiple lines so the condition, ? branch, and : branch are each on their own line.

@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 as an educational override so you can keep progressing while you practice the remaining style details. Functionally, your Person component and App usage are solid: the whole person object is passed as a prop, optional age is handled correctly, and the married / not married texts and partner titles behave as required. The main thing to improve is code style: in src/components/Person/Person.jsx, you still have a ternary on one line (for the partner role) that should be split so the condition, ? branch, and : branch are each on their own line [CHECKLIST ITEM #2]. Also in that file, the age rendering and the married vs not married partner text each combine several cases directly in JSX; define separate boolean variables (like hasAge or isSingle) first, and then use those in simpler conditionals for rendering [CHECKLIST ITEM #3]. These changes will make your code more readable and fully aligned with the checklist, but your overall structure and logic are already in a good place—nice work, and keep refining your style as you go.


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

Footnotes

  1. Rate AI review example

<p className="Person__partner">I am not married</p>
) : (
<p className="Person__partner">
{partnerName} is my {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 line violates checklist item #2: "[CODE STYLE] - Readabily is everything. Format ternary operator operands correctly - move each operand to the separate line". You currently have the whole ternary on one line; update it so the condition, ? branch, and : branch each go on their own line.

export const Person = ({ person }) => {
const { name, age, sex, isMarried, partnerName } = person;

const ageElement = age ? <p className="Person__age">I am {age}</p> : null;

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 line violates checklist item #3: "[CODE STYLE] - Avoid putting several cases to conditional rendering. Create separate variable for the condition.". Here you handle two cases (age truthy vs falsy) directly in JSX/variable initialization; instead, create a separate variable that computes the condition more explicitly (e.g. an hasAge flag or a function) and then use it for rendering.


const ageElement = age ? <p className="Person__age">I am {age}</p> : null;

const partnerContent = !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 ternary with !isMarried vs married states also violates checklist item #3: "Avoid putting several cases to conditional rendering. Create separate variable for the condition.". Consider computing a separate partnerContent or similar using a clearer condition (e.g. a boolean like isSingle) and then using a simpler conditional expression or if-statement to build the JSX.

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