Skip to content

Hw 1 #1

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

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

Hw 1 #1

wants to merge 4 commits into from

Conversation

viskosa
Copy link
Collaborator

@viskosa viskosa commented Aug 31, 2018

without copying of block


const main = props.data.users.map ((item, i) => {

let {avatarUrl, fullName, ...rest} = item;
Copy link
Member

Choose a reason for hiding this comment

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

if you are not using rest no need to create extra variable

const UserTableBody = (props) => {

return (
<table className="table table-user-information">
Copy link
Member

Choose a reason for hiding this comment

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

please fix formatting

return (
<div className="panel panel-info">

<RenderTable data = {props}/>
Copy link
Member

Choose a reason for hiding this comment

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

probably passing down ALL props not really good practice, it's much better to pass only required props.

for example

<RenderTable user={props.users}/>

or so

)
};

const RenderTable = (props) => {
Copy link
Member

Choose a reason for hiding this comment

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

that naming... I guess name it TableUsers is much better.

<div className="row">

<div>
<UserTableAvatar data = {item.avatarUrl}/>
Copy link
Member

Choose a reason for hiding this comment

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

you are using everywhere prop with naming data it says nothing about passed props, please improve such naming

<tbody>
<tr>
<td>Дата рождения</td>
<td>{props.data.birthdate}</td>
Copy link
Member

Choose a reason for hiding this comment

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

please use object destruction to extract properties from props
and rename data in props to something meaningful

const UserTableHeader = (props) => {
return (
<div className="panel-heading">
<h3 className="panel-title">{props.data}</h3>
Copy link
Member

Choose a reason for hiding this comment

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

same about props.data

const UserTableAvatar = (props) => {
return (
<div className="col-md-3 col-lg-3 " align="center">
<img src={props.data} className="pull-left"/>
Copy link
Member

Choose a reason for hiding this comment

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

please update props/data

import ReactDOM from "react-dom";
import {users} from "./components/users.js";
import 'bootstrap/dist/css/bootstrap.css';
import '../styl/index.styl';
Copy link
Member

Choose a reason for hiding this comment

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

*.styl ? :) well done

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