Skip to content

Hide language widget after first session #302

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 6 commits into from
Jun 14, 2018
Merged

Conversation

Ostrenkiy
Copy link
Contributor

Задача: #APPS-1897

Коротко для Release Notes, в формате «Сделали/Добавили/Исправили N»:
Выпилили language widget из explore при первом просмотре

Описание:
Просто добавил нужный UI и нужное поведение. Еще немного порефакторил блоки в SettingsPresenter и еще в нескольких местах по мелочи.

@Ostrenkiy Ostrenkiy added the main label Jun 6, 2018
@Ostrenkiy Ostrenkiy added this to the 1.61 milestone Jun 6, 2018
@Ostrenkiy Ostrenkiy self-assigned this Jun 6, 2018
@Ostrenkiy Ostrenkiy requested a review from kvld June 6, 2018 22:29

@IBOutlet weak var languageLabel: StepikLabel!

override func awakeFromNib() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Пустые методы.

@@ -15,4 +15,5 @@ class DefaultsContainer {
private init() {}
static let launch = LaunchDefaultsContainer()
static let personalDeadlines = PersonalDeadlinesDefaultsContainer()
static let explore = ExploreDefaultsContainer()
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 buildTitleMenuBlock(id: String, title: String) -> HeaderMenuBlock {
return HeaderMenuBlock(id: id, title: title)
enum BlockID: String {
Copy link
Contributor

Choose a reason for hiding this comment

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

Предлагаю сделать как в Profile --- делать инициализацию блоков во вью слое, а если надо иметь какую-то логику, то иметь ее в презентере.

@@ -16,6 +16,7 @@ class DefaultsStorageManager {
private let accountEmailKey = "account_email"
private let accountPasswordKey = "account_password"
private let lastCourseIdKey = "last_course_id"

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.

ну, добавился отступ лишний, бывает)

kvld
kvld previously requested changes Jun 12, 2018
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.

Так лучше, но лишнюю реализацию equatable можно попробовать убрать.


import Foundation

enum SettingsMenuBlock: String, Equatable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Тут, кстати, нет хранимых значений в case, вроде можно не определять Equatable и писать просто
enum SettingsMenuBlock: String { ... }

func changeVideoQuality(action: VideoQualityChoiceAction)
func changeCodeEditorSettings()
func changeContentLanguageSettings()
func setMenu(menuIDs: [SettingsMenuBlock])
Copy link
Contributor

Choose a reason for hiding this comment

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

Вроде же всегда было Ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Лучше делать IDs сейчас и дальше, так логичнее

@Ostrenkiy Ostrenkiy merged commit 76d7bd3 into dev Jun 14, 2018
@Ostrenkiy Ostrenkiy deleted the feature/hide-language-widget branch June 14, 2018 13:27
@Ostrenkiy Ostrenkiy mentioned this pull request Jun 18, 2018
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