Skip to content

add solution#3794

Open
ybriukhovetska wants to merge 1 commit into
mate-academy:masterfrom
ybriukhovetska:develop
Open

add solution#3794
ybriukhovetska wants to merge 1 commit into
mate-academy:masterfrom
ybriukhovetska:develop

Conversation

@ybriukhovetska

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

Your solution is approved because all core functional requirements are correctly implemented: Person is reused three times, the full person object is passed, age is omitted when missing, and the partner wording and “I am not married” behavior work as specified. The remaining issues are primarily style and checklist-related, not functionality blockers.

That said, there are a few improvements you should keep in mind for future tasks. In both App.jsx and Person.jsx, avoid using the extra className="App" wrapper and stick to the required Person, Person__name, Person__age, and Person__partner class names so the markup is fully aligned with the instructions. Also, reformat your ternary operators so each operand (condition, true branch, false branch) is on its own line, and extract conditions like person.isMarried into separate variables before using them in JSX for clearer conditional rendering [CHECKLIST ITEM #2, CHECKLIST ITEM #3]. Overall, you’ve handled the logic correctly; refining the class names and conditional formatting will improve readability and better match the prescribed style guidelines. Great job on meeting the functional requirements.


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

Footnotes

  1. Rate AI review example

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.

Checklist item #2 says: Format ternary operator operands correctly - move each operand to the separate line. Here, the conditional rendering for age uses an inline ternary-like short-circuit expression in one line; consider rewriting it across multiple lines for readability as shown in the GOOD EXAMPLE.

Comment thread src/App.jsx
@@ -1,5 +1,6 @@
import React from 'react';
import './App.scss';

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 wrapper div uses className="App", but the description only specifies using Person, Person__name, Person__age, and Person__partner, and asks to ensure class names in Person.jsx match those in App.jsx. This extra App class can violate that requirement; consider removing or renaming it to stay within the specified class set.

@@ -1 +1,15 @@
// export const Person = ({ person }) => ();
export const Person = ({ person }) => (
<div className="App">

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 outer container uses className="App", but the task requires that class names in Person.jsx match the Person-related names (Person, Person__name, Person__age, Person__partner). Consider removing or renaming this wrapper to avoid the App class here and keep only the specified Person classes.

<div className="App">
<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 line combines conditional rendering directly with a JSX fragment using short-circuit evaluation. While it hides age when missing, it doesn’t follow checklist item #3: "Avoid putting several cases to conditional rendering. Create separate variable for the condition." Consider introducing a separate variable (e.g. const showAge = Boolean(person.age);) and then using it for rendering.

<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 mixes condition and both JSX branches on a single line, which violates checklist item #2: "Format ternary operator operands correctly - move each operand to the separate line" and item #3 about avoiding complex conditional rendering. It would be clearer to extract the isMarried condition to a separate variable and format the ternary with each operand on its own line.

{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.

The nested ternary person.sex === 'm' ? 'wife' : 'husband' is written on a single line, which violates checklist item #2: "Format ternary operator operands correctly - move each operand to the separate line." Please format this ternary so that the condition, true value, and false value are on separate lines for readability.

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