-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
finished #1858
base: master
Are you sure you want to change the base?
finished #1858
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, but there is quite a bit of work to be done.
Also, according to the task, the PR descriptions must include a link to the project demo:
Replace <your_account> with your Github username in the DEMO LINK and add it to the PR description.
src/components/Person/Person.jsx
Outdated
@@ -1 +1,15 @@ | |||
// export const Person = ({ person }) => (); | |||
export const Person = ({ | |||
person: { name, age, sex, partnerName, isMarried }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such destructuring should be done in the middle of the component, not in the arguments
src/components/Person/Person.jsx
Outdated
{age && <p className="Person__age">{`I am ${age}`}</p>} | ||
<p className="Person__partner"> | ||
{isMarried | ||
? `${partnerName} is my ${sex === 'f' ? 'husband' : 'wife'}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using "magic values" (what is f ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done!
src/components/Person/Person.jsx
Outdated
<> | ||
<section className="Person"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<> | |
<section className="Person"> | |
<section className="Person"> |
React fragment is redundant here as you already have a container
src/components/Person/Person.jsx
Outdated
<h2 className="Person__name">{`My name is ${name}`}</h2> | ||
{age && <p className="Person__age">{`I am ${age}`}</p>} | ||
<p className="Person__partner"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<h2 className="Person__name">{`My name is ${name}`}</h2> | |
{age && <p className="Person__age">{`I am ${age}`}</p>} | |
<p className="Person__partner"> | |
<h2 className="Person__name">{`My name is ${name}`}</h2> | |
{age && ( | |
<p className="Person__age"> | |
{`I am ${age}`} | |
</p> | |
)} | |
<p className="Person__partner"> |
Pay attention to the formatting
src/components/Person/Person.jsx
Outdated
{age && <p className="Person__age">{`I am ${age}`}</p>} | ||
<p className="Person__partner"> | ||
{isMarried | ||
? `${partnerName} is my ${sex === SEX_FEMALE ? 'husband' : 'wife'}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change added 4 days ago, but I didn't sent them to check:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 👍
https://Fluebubble.github.io/react_person/