Skip to content

Improved Explore & Home #199

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

Merged
merged 7 commits into from
Dec 13, 2017
Merged

Improved Explore & Home #199

merged 7 commits into from
Dec 13, 2017

Conversation

Ostrenkiy
Copy link
Contributor

Задача: #APPS-1641

Коротко для Release Notes, в формате «Сделали/Добавили/Исправили N»:
Теперь экраны Home и Explore не загружают UI поток

Описание:

  • Для Course и CourseList добавил асинхронные методы для фетча с использованием NSAsynchronousFetchRequest
  • Теперь CourseList-ы на Explore экране обновляются следующим образом:
    • Если изменились ID или их порядок, то делаем refresh()
    • Если изменились ID внутри списка, то для списка обновляем listType на нужные ID и делаем onlyLocal = false
    • Если не изменились ID внутри списка, то делаем offline = false

@Ostrenkiy Ostrenkiy added the main label Dec 6, 2017
@Ostrenkiy Ostrenkiy self-assigned this Dec 6, 2017
@Ostrenkiy Ostrenkiy requested a review from kvld December 6, 2017 20:57
@Ostrenkiy Ostrenkiy added this to the 1.49 milestone Dec 6, 2017
Copy link
Contributor

@kvld kvld left a comment

Choose a reason for hiding this comment

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

Всё ок, но меня по-прежнему смущает то, как быстро разрастается CourseListPresenter (700+ строк) + множество состояний и хрупкость.

@Ostrenkiy
Copy link
Contributor Author

700+ строк из-за того, что надо поубирать еще протоколы всякие оттуда, так около 600 выходит ;)
Возможно, стоит перенести часть функционала в ListType и все-таки вынести некоторые вещи в роутинг слой.

@Ostrenkiy
Copy link
Contributor Author

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

@kvld
Copy link
Contributor

kvld commented Dec 13, 2017

Просто кажется, что логика будет расширяться, а вместе с этим и размер. В конце концов придём к тому, что это нужно будет рефакторить 🤔

@Ostrenkiy Ostrenkiy merged commit 99f130c into dev Dec 13, 2017
@Ostrenkiy Ostrenkiy deleted the feature/async-courses-fetch branch December 13, 2017 22:32
@Ostrenkiy Ostrenkiy mentioned this pull request Dec 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants