Skip to content

Conversation

ivan-magda
Copy link
Member

Задача: #1959

Описание:
Добавлена возможность просматривать уроки с теорией.

@ivan-magda ivan-magda added this to the 1.65 milestone Aug 1, 2018
@ivan-magda ivan-magda self-assigned this Aug 1, 2018
@ivan-magda ivan-magda changed the base branch from master to dev August 1, 2018 17:16
@ivan-magda ivan-magda removed the wip ⏳ label Aug 6, 2018
@ivan-magda ivan-magda requested a review from Ostrenkiy August 6, 2018 09:59
@ivan-magda ivan-magda changed the title WIP: View lessons with theory View lessons with theory Aug 6, 2018
@ivan-magda ivan-magda requested a review from kvld August 7, 2018 12:12
@ivan-magda
Copy link
Member Author

@kvld, мы решили дождаться твоего авторитетного мнения :)

Copy link
Contributor

@Ostrenkiy Ostrenkiy left a comment

Choose a reason for hiding this comment

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

Возникает много вопросов насчет целесообразности использования роутера и typed notifications, но в целом, если убрать сомнения насчет лишних усложнений, норм.
Также не нравится то, что у нас в проекте три(!) различных реализации контроллера для степа, это неоправданно много и надо с этим точно что-то делать.
Ждем комментариев от Влада, в общем.


SVProgressHUD.setDefaultMaskType(.clear)
SVProgressHUD.setMinimumDismissTimeInterval(0.5)
SVProgressHUD.setHapticsEnabled(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

ого, не знал что они это подвезли

@ivan-magda
Copy link
Member Author

Я подумал насчет роутера. Можно все значительно упростить:

  • То что я нагородил снести.
  • Оставить только одну сущность, аля RootNavigationManager, которая будет работать с навигацией.
  • Контролеры будут иметь какого-нибудь делегата закрытого протоколом, этот протокол будет реализовывать RootNavigationManager.

Все будет намного проще, в одном месте и понятно.

Пример

@ivan-magda
Copy link
Member Author

TypedNotifications привнес из этого эпизода.
Главная их цель, чтобы предоставлять type-safe работу с API NotificationCenter'а, чтобы мы могли точно знать какие к нам данные приходят/уходят, а не Any.

Я согласен, что возможно это нам и не нужно.

Вообще нужно стараться использовать NotificationCenter по-минимуму, просто в конкретном случае сложновато помечать StepTabView, как выполненную, поэтому и использовал нотификейшн центр, как и в оригинале.

Подписка на изменения:

token = NotificationCenter.default.addObserver(descriptor: Step.progressNotification) { [weak self] payload in
    guard let `self` = self,
        let stepId = self.stepId, stepId == payload.id else {
            return
    }
    
    self.setTab(selected: payload.isPassed, animated: true)
}

Отправка:

NotificationCenter.default.post(
    descriptor: Step.progressNotification,
    value: StepProgressNotificationPayload(id: step.id, isPassed: true)
)

Необходимое для работы:

struct StepProgressNotificationPayload {
    let id: Int
    let isPassed: Bool
}

extension Foundation.Notification.Name {
    public static let stepDone = Foundation.Notification.Name("StepDoneNotificationKey")
}

extension Step {
    static let progressNotification = 
        ObjectNotificationDescriptor<StepProgressNotificationPayload>(name: .stepDone)
}

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.

Типизированные нотификации – это хорошо, на самом деле, но я бы не стал завозить их сейчас, потому что это затрагивает основное приложение (а внедрить везде мы их не сможем).
Да и саму реализацию я бы тоже обсудил: возможно, не стоит отделять дескриптор и пейлоад, а просто делать для каждой нотификации отдельную сущность.
Предлагаю пока отказаться от них и постить по-старому.

Про роутинг: мне кажется, что не стоит много времени на это уделять. Здесь все равно ничего сложного нет и выигрыша от того или иного решения не получаем ведь.

return .value([])
}

return firstly {
Copy link
Contributor

Choose a reason for hiding this comment

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

Тут нужен firstly?

override func setupSubviews() {
NotificationCenter.default.addObserver(self, selector: #selector(StepTabView.stepDone(_:)), name: NSNotification.Name(rawValue: StepDoneNotificationKey), object: nil)
token = NotificationCenter.default.addObserver(descriptor: Step.progressNotification) { [weak self] payload in
guard let `self` = self,
Copy link
Contributor

Choose a reason for hiding this comment

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

strongSelf

return configuration
}()

var didFinishNavigation: (_ navigation: WKNavigation) -> Void = { _ in }
Copy link
Contributor

Choose a reason for hiding this comment

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

Можно сделать optional оба поля.

@ivan-magda
Copy link
Member Author

От нотификаций отказался в PR с практическими заданиями, теперь все прозрачно. TypedNotifications пока :trollface: .

@ivan-magda
Copy link
Member Author

ivan-magda commented Aug 13, 2018

@kvld @Ostrenkiy внёс изменения.

@ivan-magda ivan-magda merged commit 7a1023b into dev Aug 13, 2018
@ivan-magda ivan-magda deleted the feature/exam-app-theory-view branch August 13, 2018 14:37
@kvld kvld mentioned this pull request Aug 23, 2018
1 task
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.

3 participants