Skip to content

Sprint_17 - #4

Open
MiraiChan wants to merge 16 commits into
mainfrom
sprint_17
Open

Sprint_17#4
MiraiChan wants to merge 16 commits into
mainfrom
sprint_17

Conversation

@MiraiChan

Copy link
Copy Markdown
Owner

Sprint_17 done

- static strings are placed in the 'Localizable.strings' file for localization
- a group of files for Russian and English languages has been created.
…tring' function.

Created the 'Filters' button and it's UI configured.
Changed color sets for dark and light themes.
Updated Localizable.strings for filter cases.
…rackersViewController'.

Added missing NSLocalizedStrings to the main screen.
@MiraiChan

Copy link
Copy Markdown
Owner Author

Когда будете проверять, пожалуйста, оставьте более понятные инструкции к действиям, я уже обращалась к наставникам, но было непонятно. Лучше, если в коде прямо укажете, где исправить, у меня плохо получается понимать, когда вы загадками и абстрактно пишите. Спасибо

}
private func checkEmptyCategoriesScreen() {
if !viewModel.categories.isEmpty {
categoriesTableView.isHidden = true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Логику данного метода можно сократить в два раза, например так:

        let isEmptyCategories = !viewModel.categories.isEmpty
        categoriesTableView.isHidden = !isEmptyCategories
        emptyCategoryPlaceholder.isHidden = isEmptyCategories
        emptyCategoryText.isHidden = isEmptyCategories

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

}
}

func tableView(_ tableView: UITableView, contextMenuConfigurationForRowAt indexPath: IndexPath, point: CGPoint) -> UIContextMenuConfiguration? {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Метод кажется очень большим и нагруженным, может попробовать его декомпозировать и какую-то часть логики вынести в отдельный приватный метод

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok!

func addTrackerToCategory(to title: String?, tracker: Tracker) {
try? self.categoryStore.addTrackerToCategory(to: title, tracker: tracker)
func updateCategory(category: TrackerCategory?, header: String) {
try! self.categoryStore.updateCategory(category: category, header: header)

@So1ovyov So1ovyov Jan 3, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Force try было бы круто не использовать и поменять на опциональный, как это сделано ниже

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

backgroundColor = .clear
clipsToBounds = true

addSubview(borderView)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Здесь можно также использовать forEach чтобы сократить количество повторяющегося кода

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

final class StatisticsViewController: UIViewController {

let cellReuseIdentifier = "StatisticViewController"
var trackersViewController: TrackersViewController?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Точно ли есть необходимость так завязываться на ViewController ? Может надежнее его передавать через инициализатор ?
Возможно из-за этого подхода на экране всегда показываются ячейки со счетчиками, даже когда трекеров нет

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

О, спасибо, помогло пофиксить.

private func showPlaceholder() {
guard let trackersViewController = trackersViewController else { return }

if trackersViewController.completedTrackers.count > 0 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Здесь также можно сократить код в два раза, как предлагалось выше в похожем кейсе

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

private var category: String? = nil

private let addCategoryViewController = CategoryViewController()
//private var category: String? = nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Давай избавимся от закомментированного кода

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

trackerDescription.text = tracker.name

self.pinnedTracker.isHidden = tracker.pinned ? false : true
print("Preview", tracker.pinned)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Print для дебага тоже можно удалить

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Done.

@So1ovyov

So1ovyov commented Jan 3, 2024

Copy link
Copy Markdown

@MiraiChan не совсем понял комментарий по поводу инструкций к действиям. По предыдущим ПР-ам и ревью, кажется что описывались проблемы очень конкретно и понятно, с указанием в коде, где нужно поправить.
Если же речь про комментарии на платформе, то подразумевается, что в коде в нескольких местах указали, что нужно поправить, а дальше не указывали, так как скорее всего такие ошибки несколько раз повторяются. Поэтому чтобы в самом ПР одно и тоже не писать (не спамить) пишется об этом в целом.
Надеюсь, что я со своей стороны указал все моменты подробно и с понятным описанием, если будут какие-то вопросы, то обязательно обращайся !
У нас нет цели завалить, докопаться, мы стремимся научить и подсказать, что можно сделать лучше.

…omments.

Force unwrap replaced with optionals.
…ted initializer (init(context:)) when initializing TrackerCategoryStore in the convenience initializer (init()).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants