-
Notifications
You must be signed in to change notification settings - Fork 35
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
Notifications center #134
Notifications center #134
Conversation
Надо бы dev в ветку подмерджить и посмотреть, что там с тестами. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В целом хорошо. Давай пока отправим это в бету, а завтра выпустим итоговый вариант.
Stepic/DeepLinkRouter.swift
Outdated
@@ -213,4 +225,40 @@ class DeepLinkRouter { | |||
) | |||
|
|||
} | |||
|
|||
static func routeToDiscussionWithId(_ lessonId: Int, stepId: Int, discussionId: Int, completion: @escaping ([UIViewController]) -> Void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
вот в этом файле, конечно, адовый ад.
надо будет отрефакторить это всё потом, чтобы на промисах и, скорее всего, в отдельные классы выделить роутинг на различный контент, чтобы можно было удобно конфигурировать и расширять поведение роутинга.
@@ -157,6 +157,11 @@ class NewProfileViewController: MenuViewController, ProfileView { | |||
navigationController?.pushViewController(vc, animated: true) | |||
} | |||
|
|||
func navigateToNotifications() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это нигде не используется. Не знаю, стоит ли здесь и в ProfilePresenter
оставлять.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не стоит, это осталось случайно.
import CoreData | ||
|
||
extension Notification { | ||
// FIXME: CREATE GENERIC CLASS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
хорошая идея
Stepic/Notification.swift
Outdated
|
||
enum NotificationType: String { | ||
var localizedName: String { | ||
let localizedNames: [NotificationType: String] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А чем это лучше switch self
-а ?
switch self
без default
statement-а позволит тебе не забыть добавить локализованное имя при добавлении нового типа нотификаций, или как-то сгруппировать старые, например.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Справедливо, осталось от какого-то другого кейса просто.
// | ||
|
||
import Foundation | ||
|
||
/* | ||
Extracts information from the given notification | ||
*/ | ||
class NotificationDataExtractor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хорошо отрефакторил класс, круто!
import Alamofire | ||
import SwiftyJSON | ||
|
||
class NotificationsAPI { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хорошо было бы заюзать PromiseKit для запросов в этом классе. (можно потом, в feature/new-home добавил PromiseKit обертку над APIEndpoint
)
} | ||
|
||
fileprivate func loadData(page: Int, in section: NotificationsSection) -> Promise<(Bool, NotificationViewDataStruct)> { | ||
// FIXME: temporary wrapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
вот это такое себе, но пока можно оставить
// FIXME: temporary wrapper | ||
func retrieve(page: Int, notificationType: NotificationType?) -> Promise<(Meta, [Notification])> { | ||
return Promise { fulfill, reject in | ||
// TODO: ugly performRequest() call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
думаю, можно будет написать PromiseKit-обертку над performRequest(), чтобы все выглядело посимпатичнее. (не сейчас)
Задача: #APPS-1481
Коротко для Release Notes, в формате «Сделали/Добавили/Исправили N»:
Добавили раздел с уведомлениями
Описание:
Новый раздел с уведомлениями.
TODO