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

initial commit #1588

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

Conversation

maksym-mishchanchuk
Copy link

Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

Good progress 👍
But some comments below

return (
<>
<section className="Person">
<h2 className="Person__name">{ `My name is ${name}` }</h2>

Choose a reason for hiding this comment

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

Suggested change
<h2 className="Person__name">{ `My name is ${name}` }</h2>
<h2 className="Person__name">{`My name is ${name}`}</h2>

Comment on lines 17 to 23
{age
&& (
<p className="Person__age">
{`I am ${age}`}
</p>
)
}

Choose a reason for hiding this comment

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

Suggested change
{age
&& (
<p className="Person__age">
{`I am ${age}`}
</p>
)
}
{age && (
<p className="Person__age">
{`I am ${age}`}
</p>
)}

Comment on lines 27 to 29
{sex === 'f'
? `${partnerName} is my husband`
: `${partnerName} is my wife`

Choose a reason for hiding this comment

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

Try not to nest ternary, create constant for this

export function Person({ person }) {
const {
name,
age = null,

Choose a reason for hiding this comment

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

Suggested change
age = null,
age,

}

{isMarried ? (
<p className="Person__partner">

Choose a reason for hiding this comment

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

This tag is the same for both variants, you don't need to duplicate it

Copy link

@polosanya polosanya left a comment

Choose a reason for hiding this comment

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

Almost done!

Comment on lines 17 to 18
<>
<section className="Person">

Choose a reason for hiding this comment

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

Suggested change
<>
<section className="Person">
<section className="Person">

There is no need to use React fragment here as you already have a container

partnerName,
} = person;

const whoIsPartner = sex === 'f'

Choose a reason for hiding this comment

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

Suggested change
const whoIsPartner = sex === 'f'
const getPartner = sex === 'f'

image

Comment on lines 21 to 26
{age
&& (
<p className="Person__age">
{`I am ${age}`}
</p>
)}

Choose a reason for hiding this comment

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

Suggested change
{age
&& (
<p className="Person__age">
{`I am ${age}`}
</p>
)}
{age && (
<p className="Person__age">
{`I am ${age}`}
</p>
)}

)}

<p className="Person__partner">
{isMarried ? `${whoIsPartner}` : 'I am not married'}

Choose a reason for hiding this comment

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

Suggested change
{isMarried ? `${whoIsPartner}` : 'I am not married'}
{isMarried
? `${whoIsPartner}`
: 'I am not married'
}

image

<section className="Person">
<h2 className="Person__name">{`My name is ${name}`}</h2>

{age && (

Choose a reason for hiding this comment

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

I guess if age is gonna be 0 it's not gonna work correctly, I mean age 0 is not correctly itself, but react will probably just render '0' on the page, so we have to convert it to boolean to make sure it works just fine, we can achieve it by using !! or Boolean

partnerName,
} = person;

const getPartner = sex === 'f'

Choose a reason for hiding this comment

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

get sounds like an action, good prefix for function, but not variable

I'd like you to name it as partnerInfo or something like this, you decide

@tiserett
Copy link

btw, demo link redirects to blank page

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.

4 participants