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

ДЗ №8 (1) #196

Open
wants to merge 11 commits into
base: necrolyss/homework-8
Choose a base branch
from
Open

Conversation

necrolyss
Copy link

No description provided.

}
}

const AuthConsumerHOC = WrappedComponent =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

не забудь убрать весь этот файл, он не нужен, тут много лишней логики, которая не нужна)

class UserPage extends Component {

fetchData = () => {
this.props.getLoginRequest();
Copy link
Collaborator

Choose a reason for hiding this comment

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

ты должен научить эту компоненту понимать, есть ли имя пользователя и в зависимости от этого отправлять 2 разных экшена getLoginRequest или getUserInfoRequest

});

export { getLoginRequest, getLoginSuccess, getLoginFailure,
getUserInfoRequest, getUserInfoSuccess, getUserInfoFailure };
Copy link
Collaborator

Choose a reason for hiding this comment

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

оО тут должно быть 4 экшена, а у тебя целых 6

getLoginFailure,
getUserInfoRequest,
getUserInfoSuccess,
getUserInfoFailure
Copy link
Collaborator

Choose a reason for hiding this comment

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

4 экшена


const userInfo = handleActions(
{
[getUserInfoSuccess.toString()]: (_state, action) => action.payload.data
Copy link
Collaborator

Choose a reason for hiding this comment

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

нужно убрать во всех редьюсерах вот тут вызов toString()

} from "../ducks/user-actions";

function* fetchUserWatch(action) {
while (true) {
Copy link
Collaborator

@jpiggg jpiggg Jul 14, 2018

Choose a reason for hiding this comment

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

Цикл в данном случае не нужен.
Он был нужен конкретно для саги авторизации, там более сложная логика, по которой тебе нужно было научиться понимать разные случаи для пользователя такие, как: пользователь перешел на любую страницу и нужно проверить авторизован ли он или нет

В саге пользователя тебе нужно запускать сагу только при получении конкретных экшенов и делать после этого последовательную работу 1 раз, зачем тут цикл?

Copy link
Collaborator

Choose a reason for hiding this comment

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

советую пересмотреть вебинар Артема про саги, думаю, что это будет полезно

while (true) {
try {
const userLookupResult = yield call(getTokenOwner);
let loginSuccessResult = getLoginSuccess(userLookupResult);
Copy link
Collaborator

@jpiggg jpiggg Jul 14, 2018

Choose a reason for hiding this comment

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

Пожалуйста, называй переменные констистентно:) Очень трудно читать код, когда у тебя сага, относящаяся к пользователю называется searchRequestWatch - при чем тут поиск вообще?
Приходится тратить гораздо больше времени на чтение кода, когда нет констистентности в именовании


let loginSuccessResult - это можно было бы назвать просто response

}

function* searchRequestWatch() {
yield takeLatest(getLoginRequest, fetchUserWatch);
Copy link
Collaborator

Choose a reason for hiding this comment

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

эта сага должна реагировать на 2 экшена, на проверку владельца токена ( getLoginRequest - его лучше назвать fetchTokenOwnerRequest - тогда сразу понятно, что тут происходит) и второй экшен getUserInfoRequest. В try/catch ты должен в замисимости от того или иного экшена вызывать эффект call (в одном случае в него передавать getTokenOwner, а в другом getUserInformation)

try {
const userLookupResult = yield call(getTokenOwner);
let loginSuccessResult = getLoginSuccess(userLookupResult);
yield put(loginSuccessResult);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Посмотри внимательно на эту строку, неправильно используешь эффект put
В этот эффект ты должен передать экшен (на строке 20 этого файла ты правильно делаешь)

null
);

export const LOADING_STATE = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

этой константы не должно быть

const loadingState = handleActions(
{
[getFollowersRequest.toString()]: () => LOADING_STATE.loading,
[getFollowersSuccess.toString()]: () => LOADING_STATE.success,
Copy link
Collaborator

Choose a reason for hiding this comment

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

зачем тут константы?
я уже, кажется, писала про toString(), по крайней мере говорила про это на групповом созвоне в прошлое воскресенье. так писать не надо

export const isFetched = handleActions(
  {
    [fetchFollowersRequest]: () => false,
    [fetchFollowersSuccess]: () => true,
    [fetchFollowersFailure]: () => true
  },
  false
);

вот так должно быть, просто флаги, не нужно усложнять логику)

Про именование тоже говорила на созвоне, этот редьюсер лучше назвать isFetched

export const isFetching = state => {
let currentState = state.login.loadingState;
return currentState === LOADING_STATE.idle
|| currentState === LOADING_STATE.loading;
Copy link
Collaborator

@jpiggg jpiggg Jul 20, 2018

Choose a reason for hiding this comment

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

используй флаги

export const isFetching = handleActions(
  {
    [fetchFollowersRequest]: () => true,
    [fetchFollowersSuccess]: () => false,
    [fetchFollowersFailure]: () => false
  },
  false
);

@dex157
Copy link
Owner

dex157 commented Jul 31, 2018

У тебя беспорядок со структурой папок и файлов в коде:

  • В src/ лежит App.js который не нужен в проекте

  • В src/ducks беспорядок, network сделан папкой, остальные файлы сгруппированы через дефис. - Нужно держать все в одном стиле, я помню что network скинул тебе я, но если ты хочешь свою структуру файлов, то нужно было привести network к ней.

  • Именование модулей через дефис бессмысленно, нужно делать через папки

  • Компоненты без папок, с 5 компонентами у тебя в одной папке 12 файлов, это быстро приведет к тому, что ориентироваться в папке станет невозможно, нужно держать структуру в чистоте.

</p>
<input
onChange={this.handleChange}
onKeyPress={(e) => this.handleKeyPress(e)}
Copy link
Owner

Choose a reason for hiding this comment

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

Почему тут (e) => this.handleKetPress(e)? нужно просто onKeyPress={this.handleKeyPress}

const payload = [
{
data: { login: "test1", id: 1 },
data: { login: "test2", id: 2 }
Copy link
Owner

@dex157 dex157 Jul 31, 2018

Choose a reason for hiding this comment

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

Тут второй payload.data затрет первый payload.data, payload будет иметь структуру

[
  {
    data: {
      login: "test2",
      id: 2
    }
  }
]

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