Skip to content

Conversation

@rybakovi
Copy link
Contributor

@rybakovi rybakovi commented Oct 2, 2020

No description provided.

@rybakovi rybakovi requested a review from maxbach as a code owner October 2, 2020 10:50
Comment on lines 11 to 13
У MviViewModel есть один абстрактный метод для обработки внешних событий пользователя.
События должны наследоваться от интерфейса-маркера ViewAction. Также у MviViewModel есть
livedata, которая держит в себе single state экрана. То есть у MviViewModel есть один вход и один выход.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Какой метод для обрабатки внешних событий?
  2. Он не абстрактный
  3. По аналогии с ViewAction от чего наследуется SingleState экрана?

Comment on lines 17 to 19
Помимо ограничений в количестве потока данных MviViewModel использует AssistedInject в конструктор для
получения inititalState через конструктор и механизм для сохранения данных экрана в Bundle - SavedStateHandle.
AssistedInject используется для передачи SavedStateHandle из фрагмента, который подключает ViewModel.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Помимо ограничений в количестве потока данных - лишнее здесь
  2. Если я правильно понял, то речь идёт о navArgs, которые прокидываются из фрагмента во MviViewModel и доступны внутри неё


### MviStoreViewModel

MviStoreViewModel наследуется от MviViewModel. Единственная ценность этого класса - он хранит в себе список Store, передает Action внутрь Store
Copy link
Contributor

Choose a reason for hiding this comment

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

передает Action внутрь Store - запятая

### MviStoreViewModel

MviStoreViewModel наследуется от MviViewModel. Единственная ценность этого класса - он хранит в себе список Store, передает Action внутрь Store
комбинирует стейты всех сторов в ожин большой стейт.
Copy link
Contributor

Choose a reason for hiding this comment

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

в один

- [Доклад про MVI от Сергея Рябова](https://youtu.be/hBkQkjWnAjg)
- [Доклад про эволюцию презентационных паттернов](https://youtu.be/J0YPKcDKumk)
- [Доклад про расширяемую архитектуру в Lyft](https://www.youtube.com/watch?v=_cFHtjIWjCc)
- [Презентация доклада со внутреннего митапа](Доклад Илюхи)
Copy link
Contributor

Choose a reason for hiding this comment

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

добавить линк

Для использования Single-Activity подхода возникают ситуации, когда несколько экранов нужно объединить одной родительской сущностью.
Объединение необходимо для хранения общего dagger компонента на flow или организации собственной навигации.
Раньше можно было использовать Activity. Но в Single-Activity должно быть только одно активити на приложение. В такой архитектуре можно использовать
parent fragment для всех экран - FlowFragment. FlowFragment обладает своей навигацией. Для навигации используется childFragmentManager этого фрагмента.
Copy link
Contributor

Choose a reason for hiding this comment

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

для всех экранов


### Почему всегда не использовать MviStoreViewModel

Реализация полного MVI требует много кода. Для простых экранов, то есть для 85 процентов экранов рекомендуется использовать MviViewModel.

Choose a reason for hiding this comment

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

Suggested change
Реализация полного MVI требует много кода. Для простых экранов, то есть для 85 процентов экранов рекомендуется использовать MviViewModel.
Реализация полного MVI требует много кода. Для простых экранов (т.е для большиства экранов) рекомендуется использовать MviViewModel.


// TODO: перенести в список
// Переменная, которая отвечает за отображение лоадера в конце списка. Если переменная равна true, лоадер не будет показан
internal var fullData = false

Choose a reason for hiding this comment

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

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

object RefreshFailed : Error()
}

// Способы обработки ошибки

Choose a reason for hiding this comment

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

Способ реакции на ошибку

}

sealed class Error {
// Ошибка загрузки новой страницы

Choose a reason for hiding this comment

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

бесполезный комментарий


sealed class Error {
// Ошибка загрузки новой страницы
object NewPageFailed : Error()

Choose a reason for hiding this comment

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

NewPageLoadingFailed

// Ошибка загрузки новой страницы
object NewPageFailed : Error()

// Ошибка обновления страницы

Choose a reason for hiding this comment

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

бесполезный комментарий

object NewPageFailed : Error()

// Ошибка обновления страницы
object RefreshFailed : Error()

Choose a reason for hiding this comment

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

RefreshPageFailed

import javax.inject.Inject

/**
* Base parent fragment for fragments of hole feature. FlowFragment has own navigator based on childFragmentManager.

Choose a reason for hiding this comment

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

Наверное все-таки не hole

}))
}

// При байндинге одного из последних элементов списка запускается загрузка следующей страницы

Choose a reason for hiding this comment

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

Комментарий сомнительной полезности

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.

5 participants