Skip to content
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

react person #1559

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

alina-isakova
Copy link

Comment on lines 15 to 19
{person.isMarried === true ? (
`${person.partnerName} is my ${person.sex === 'm' ? 'wife' : 'husband'}`
) : (
'I am not married'
)}

Choose a reason for hiding this comment

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

  1. move this conditions to a variable to increase readability
  2. person.isMarried === true ? is the same as just person.isMarried ?

Comment on lines 1 to 2
export const Person = ({ person }) => (
<section className="Person">

Choose a reason for hiding this comment

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

it's a good practice to use object destructing

Suggested change
export const Person = ({ person }) => (
<section className="Person">
export const Person = ({ person }) => {
const {...} = person
return (
<section className="Person">

src/App.jsx Outdated
Comment on lines 29 to 31
<Person
person={misha}
/>

Choose a reason for hiding this comment

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

no needs to write from a new line since you have only one prop.
Apply for other cases below

Suggested change
<Person
person={misha}
/>
<Person person={misha} />

Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

You've got this, keep pushing forward!

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.

3 participants