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

New home #152

Merged
merged 48 commits into from
Oct 31, 2017
Merged

New home #152

merged 48 commits into from
Oct 31, 2017

Conversation

Ostrenkiy
Copy link
Contributor

@Ostrenkiy Ostrenkiy commented Oct 27, 2017

Задача: #APPS-1525

Коротко для Release Notes, в формате «Сделали/Добавили/Исправили N»:
Убрали экран "My Courses". Добавили экран "Home".

Описание:
Сделаны следующие абстракции:

  • CourseList - основная логика в CourseListPresenter. Также переиспользуется CourseListViewController, будучи родителем CourseListHorizontalViewController и CourseListVerticalViewController. В CourseListViewController попытался вынести все общие методы для горизонтальных и вертикальных отображений. Также поддерживаются различные цветовые режимы (подробнее в enum-е CouseListColorMode) и слежение за подпиской на курсы через NotificationCenter (CourseSubscriptionManager). Все запросы здесь делаются через PromiseKit и инкапсулированы в enum-е CourseListType (так как по факту запросы на получения курсов различны лишь для различных типов списков курсов). Возможно, существуют способы получше сделать логику для запроса прогрессов и рейтингов.

  • HomeScreen - немного логики в HomeScreenPresenter, всю нагрузку делегируем в CourseList-ы. Для отображения всех CourseList-ов в HomeScreenViewController используем UIStackView. Для корректной работы LastStepWidget подписываем делегат в CourseList c типом .enrolled и по окончании загрузки курсов пользователя отображаем виджет с переходом к последнему шагу.

Demo
home-demo

TODO

  • Titles
  • Empty states
  • Error states
  • Localizations
  • Courses count
  • Course Main Button blinking
  • Update courses when internet is reachable (if they were not updated) - если юзер зашел в оффлайн режиме, у него отображаются старые курсы. При появлении интернета надо незаметно рефрешить список. Хорошо бы в дальнейшем продумать механизм рефреша курсов.
  • Continue Learning after subscribing/unsubscribing - пока добавляем в Continue Learning блок последний курс, на который юзер записался/отписался. В дальнейшем будем динамически рассчитывать last_active для курса/секции/степа и при каждом появлении виждета сортировать курсы по последней активности.
  • Подумать, как можно реюзать HomeScreenViewController так, чтобы весь UI для Catalog экрана был доступен "из коробки" и не приходилось переписывать весь код.

@Ostrenkiy Ostrenkiy added this to the 1.46 milestone Oct 27, 2017
@Ostrenkiy Ostrenkiy self-assigned this Oct 27, 2017
@Ostrenkiy Ostrenkiy requested a review from kvld October 27, 2017 19:11
@Ostrenkiy Ostrenkiy changed the title Feature/new home New home Oct 27, 2017
@kvld
Copy link
Contributor

kvld commented Oct 31, 2017

Визуальные проблемы:

  1. Высота вьюшки Continue learning после поворота неадекватна
  2. Локализация делает отображение прогресса неинформативным
  3. "Бегущий" градиент не очень выглядит на тёмном :(
  4. На 5.5 устройстве фризит в таком состоянии после открытия на несколько секунд

.swiftlint.yml Outdated
- colon
- unneeded_break_in_switch
- unneeded_parentheses_in_closure_argument
- switch_case_alignment
Copy link
Contributor

Choose a reason for hiding this comment

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

Отключили какие-то правила? оО

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Да. Там вышло обновление swiftlint-а и все сломалось. Отдельным PR-ом будем разбираться с новыми правилами.


enum RetrieveError: Error {
case connectionError, badStatus, cancelled

init(error: Error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Вот это хорошо. Правда дальше снова есть код в духе if e.code == -999 { ... }.
Ну, и как ты уже упоминал, неплохо минимизировать кол-во enum'ов с ошибками.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

А как без разбора кодов в свитче делать?
Я интеллектуальнее способа не придумал :)


static let sharedManager = CourseSubscriptionManager()

let courseSubscribedNotificationName = NSNotification.Name(rawValue: "CourseSubscribedNotification")
Copy link
Contributor

Choose a reason for hiding this comment

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

У нас вроде бы скопилось достаточно таких мест в коде. Может сделаем extension для NSNotification.Name и всё туда перенесём?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Отдельным PR-ом тогда

return (deletedCourses.count + addedCourses.count) > 0
}

func filterRepetitions(arr: [Course]) -> [Course] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Возможно, это тоже нужно вынести в extension к Array (если фильтрация дубликатов ещё где-то используется).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Там идет сравнение курсов по id.
Лучше сделать это отдельно, только для JSONInitializable объектов видимо (или сделать что-нибудь с Equatable для JSONInitializable чтобы все было хорошо)
Сейчас что-то не хочется заморачиваться. Хочется бету выпустить.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

}

func initBlocks() {
let showController: (UIViewController) -> Void = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Это никак нельзя делать на уровне view, а не в презентере?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Обсудим введение Router слоя и отрефакторим в следующий релиз тогда

private var isContinueLearningWidgetPresented: Bool = false

private func presentLastStep(for course: Course) {
guard let widgetData = ContinueLearningWidgetData(course: course, navigation: view?.getNavigation()) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

То же самое. Точно в презентере должна быть эта логика? 🤔

}

private func checkIsGoodForLastStep(course: Course) -> Bool {
return course.scheduleType != "ended" && course.scheduleType != "upcoming" && !course.sectionsArray.isEmpty
Copy link
Contributor

Choose a reason for hiding this comment

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

Превратить scheduleType в enum?

let imageURL: String
let continueLearningAction: (() -> Void)?

init?(course: Course, navigation: UINavigationController?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ссылка на контроллер в DTO?

@@ -67,6 +67,15 @@ extension UILabel {
}
}

extension UILabel {
var numberOfVisibleLines: Int {
let textSize = CGSize(width: CGFloat(self.frame.size.width), height: CGFloat(MAXFLOAT))
Copy link
Contributor

Choose a reason for hiding this comment

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

CGFloat(MAXFLOAT) -> CGFloat.greatestFiniteMagnitude ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

согласен, пофикшу

@@ -331,5 +331,20 @@ NoNotifications = "No notifications";
AnonymousNotificationsTitle = "Anonymous users can't receive notifications";
SignInToHaveNotifications = "Sign in to be able to receive notifications";

ContinueLearningWidgetButtonTitle = "Continue learning"; //+
Copy link
Contributor

Choose a reason for hiding this comment

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

А что за комменты?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

забыл потереть

@Ostrenkiy
Copy link
Contributor Author

Ostrenkiy commented Oct 31, 2017

Merge checklist

  • Высота Continue Learning виджета в лендскейпе
  • Локализация для отображения прогресса в Continue Learning виджете
  • Градиент
  • Проблемы с лейаутом на 5.5 дюймовом экране
  • CGFloat(MAXFLOAT) -> CGFloat.greatestFiniteMagnitude
  • Комментарии в локализациях

@Ostrenkiy Ostrenkiy changed the base branch from master to dev October 31, 2017 21:05
@Ostrenkiy Ostrenkiy merged commit 47e0336 into dev Oct 31, 2017
@Ostrenkiy Ostrenkiy deleted the feature/new-home branch October 31, 2017 21:06
@Ostrenkiy Ostrenkiy mentioned this pull request Nov 1, 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