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

Features/users #8

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from
Open

Features/users #8

wants to merge 13 commits into from

Conversation

pronovaso
Copy link
Collaborator

No description provided.

@pronovaso pronovaso requested a review from adostal July 31, 2018 11:49
Copy link
Contributor

@adostal adostal left a comment

Choose a reason for hiding this comment

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

Viz komentare

readonly contacts: {};
}

export const SimpleModal: React.SFC<Props> = ({firstName, lastName, email, phoneNumber, isOpen, handleClose}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Soubor by se mel jmenovat jako komponenta, tedy SimpleModal.tsx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opraveno

@@ -1,8 +1,107 @@
import * as React from 'react';
import {WithAdminProps} from '@client/with/withAdmin';
import {Paper, Table, TableHead, TableRow, TableCell, TableBody, Button} from '../../../../node_modules/@material-ui/core';
import AddIcon from '@material-ui/icons/Add';
Copy link
Contributor

Choose a reason for hiding this comment

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

Ikony jdou importovat i takto:

import {Add, Delete as DeleteIcon} from '@material-ui/icons'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opraveno

@@ -1,8 +1,107 @@
import * as React from 'react';
import {WithAdminProps} from '@client/with/withAdmin';
import {Paper, Table, TableHead, TableRow, TableCell, TableBody, Button} from '../../../../node_modules/@material-ui/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

tady je spatne import, mel by byt:

import {...} from '@material-ui/core'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opraveno

},
};
const initialState = {
contacts: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Tohle je fajn, akorat ti chybi datovy typ pro kontakty:

interface ContactModel {
  id: number;
  firstName: string;
  lastName: string;
  email: string;
  // tady by teoreticky mohlo bbyt cislo i string; TS ma moznost napsat takto, ze promenna muze byt dvou datovych typu
  phoneNumber: number | string;
}

const initialState = {
 contacts: [...] as ContactModel[]
}

export class UsersIndexPage extends React.Component<WithAdminProps, typeof initialState> {
readonly state = initialState;

handleOpen = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

ty handle metody by mely byt psany jako: handleOn..., tedy handleOnOpen, handleOnClose, apod

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

jasná,ukládám

<Button variant="fab" color="primary" aria-label="Add" mini style={styles.button} onClick={this.handleOpen}>
<AddIcon />
</Button>
<SimpleModal isOpen={isOpen} handleClose={this.handleClose} contacts={contacts} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Tohle ti hlasi chybu, protoze SimpleModal ma na props toto:

interface Props {
    readonly isOpen: boolean;
    readonly handleClose: () => void;
    readonly firstName: string;
    readonly lastName: string;
    readonly email: string;
    readonly phoneNumber: number;
    readonly children?: React.ReactNode;
    readonly contacts: {};
}

V tom pripade bys tady musel mit toto:

<SimpleModal isOpen={isOpen} handleClose={this.handleClose} contacts={contacts} firstName="" lastName="" ... />

Tedy dopsat ostatni. Ale lepsi je to udelat pres vnoreny objekt, viz komentar u modal.tsx u props


export const SimpleModal: React.SFC<Props> = ({firstName, lastName, email, phoneNumber, isOpen, handleClose}) => {
return (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ten div uz tady nepotrebujes, pokud nepocitas, ze krome modal okna zde nebude jeste neco jineho

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opraveno

import TextField from '../../../../node_modules/@material-ui/core/TextField';
import Button from '../../../../node_modules/@material-ui/core/Button';

interface Props {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ten props bych napsal jinak. Mas tady rozebrane jednotlive atributy uzivatele, coz by se dalo napsat radeji takto:

interface Props {
    readonly isOpen: boolean;
    readonly handleClose: () => void;
    // jednotlive atributy bych dal do vlastniho objektu
    readonly firstName: string;
    readonly lastName: string;
    readonly email: string;
    readonly phoneNumber: number;
    // children uz nepotrebujes, pridava ho samotny typ React.SFC
    readonly children?: React.ReactNode;
    // ContactModel je v druhem souboru, takze bud to dat do vlastniho a nebo v nem musis mit export
    readonly contacts: {};
}

// vysledek
interface UserModel {
    readonly id: number;
    readonly firstName: string;
    readonly lastName: string;
    readonly email: string;
    readonly phoneNumber: number;
}

interface Props {
    readonly isOpen: boolean;
    readonly handleClose: () => void;
    // pouzijes UserModel a Pick, ktery vynecha povinne ID, ktere tady nechces
    readonly user: Pick<UserModel, 'id'>;
    readonly contacts: ContactModel[];
}

na strane JSX pouziti to bude takto:

<SimpleModal isOpen={isOpen} handleClose={this.handleClose} contacts={contacts} user={user} />

ten user nekde musis inicializovat, takze bud tady a nebo az v SimpleModal, cimz ho pak nepotrebujes vubec jako atribut od parenta UsersIndexPage.tsx

@pronovaso pronovaso requested a review from adostal August 3, 2018 16:32
interface Props {
contacts: ContactModel[];
deleteContact: (id: number) => () => void;
handleOnId: (id: number) => () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tenhle nazev moc nerika, co vlastne dela. Chtelo by to jiny. Navic se to pouziva tak, ze v props, coz jde do atributu je to takto: onClick, onClickEdit, onClickDelete, apod. V pripade implementace se pak pouziva nazev: handleOnClickDelete, apod.

@pronovaso pronovaso requested a review from adostal August 8, 2018 14:44

interface Props {
contact: ContactModel;
deleteContact: (id: number) => () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tady bys nemel potrebovat funkci, co vraci funkci

@pronovaso pronovaso requested a review from adostal August 15, 2018 12:40
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