Skip to content

React#2

Open
vmstarchenko wants to merge 16 commits intohse2016:masterfrom
vmstarchenko:master
Open

React#2
vmstarchenko wants to merge 16 commits intohse2016:masterfrom
vmstarchenko:master

Conversation

@vmstarchenko
Copy link
Copy Markdown

Всё работает кроме фокуса на input при редактировании тудушки.
Есть пара костылей, которые не удалось сделать нормально.

@vmstarchenko
Copy link
Copy Markdown
Author

vmstarchenko commented Dec 11, 2016

Выложил на heroku
Url: https://vmstarchenko-react-homework.herokuapp.com/

@vmstarchenko
Copy link
Copy Markdown
Author

Пофиксил фокус, Добавил сохранение state в localStorage.
Теперь совсем всё готово.

@EAndreyF
Copy link
Copy Markdown
Contributor

А ты css откуда брал? Сейчас это всё лежит совсем не по компонентам, если брал как и в прошлом задании из npm модулей, то так и стоило оставить

Copy link
Copy Markdown
Contributor

@EAndreyF EAndreyF left a comment

Choose a reason for hiding this comment

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

В целом не плохо, но слишком большие компоненты, и меняешь пропсы, передаешь мало хендлеров, неявная работа со state

if (!mainList) {
mainList = [{todos : []}];
state.set(mainList);
state.save();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

сделал бы через chain выглядело бы лучше

class MainList extends React.Component {

addTodoList() {
this.props.data.push({todos:[], completed: false});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Нельзя менять пропсы) Ты можешь менять только state


addTodoList() {
this.props.data.push({todos:[], completed: false});
this.setState({data: this.props.data});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Зачем ты сетишь стейт, если нигде из него не читаешь? Только для того чтобы вызвался перерендеринг

completedNumber = 0;

let todoTemplates,
calculateTodos = this.calculateTodos.bind(this);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

обычно такой биндинг делают в конструкторе, а то у тебя при каждом рендеринге будет биндинг

completedNumber = 0;

let todoTemplates,
calculateTodos = this.calculateTodos.bind(this);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

обычно такой биндинг делают в конструкторе, а то у тебя при каждом рендеринге будет биндинг


let Todo = require("./Todo").Todo;

class TodoList extends React.Component {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

У тебя слишком много намешано в этом компоненте и футер и добавление тудушек и футер с фильтрами. Стоит разбивать гораздо мельче

<TodoList
key={index}
data={item}
handlers={{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Хорошо что ты передаешь хэндлеры в глубь, но слишком мало) У тебя очень много изменений пропсов

this.setState({
data: this.props.data
});
state.save();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Интересно, у тебя нигде нет изменения state, но ты его сохраняешь, как-то не явно всё

toggleCompleted() {
let data = this.props.data;
data.completed = !data.completed;
this.setState({data: data});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Плохое название переменной data - у тебя много где эта переменная используется, становится непонятно, что там лежит

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