Conversation
ykws
left a comment
ykws
left a comment
There was a problem hiding this comment.
詳細画面の要件が特に明記されていないので、やりづらかったですかね。
リスト表示から詳細へは選択した天気予報のデータを渡すだけで通信せずに、元の天気予報画面を表示する形式も、実際のアプリではよくある遷移パターンの一つになります。
今回の実装のように、詳細画面で詳細な情報を取得して表示するパターンもあり得ますが、画面遷移の度に通信するのは高コストなので、キャッシュ表示を組み合わせて展開されると思います。
全部見れていないですが、一旦コメントです。
ぱっと見、リスト表示に関しては良さそうですね。
| override func awakeFromNib() { | ||
| super.awakeFromNib() | ||
| } | ||
|
|
||
| override func setSelected(_ selected: Bool, animated: Bool) { | ||
| super.setSelected(selected, animated: animated) | ||
| } | ||
|
|
There was a problem hiding this comment.
super を呼ぶだけなら省略して良いです。
| override func awakeFromNib() { | |
| super.awakeFromNib() | |
| } | |
| override func setSelected(_ selected: Bool, animated: Bool) { | |
| super.setSelected(selected, animated: animated) | |
| } | |
| override func awakeFromNib() { | ||
| super.awakeFromNib() | ||
| } | ||
|
|
||
| override func setSelected(_ selected: Bool, animated: Bool) { | ||
| super.setSelected(selected, animated: animated) | ||
| } | ||
|
|
There was a problem hiding this comment.
super を呼ぶだけなら省略して良いです。
| override func awakeFromNib() { | |
| super.awakeFromNib() | |
| } | |
| override func setSelected(_ selected: Bool, animated: Bool) { | |
| super.setSelected(selected, animated: animated) | |
| } | |
| super.setSelected(selected, animated: animated) | ||
| } | ||
|
|
||
| func configure(weatherResponses: WeatherListResponse) { |
There was a problem hiding this comment.
複数ではない？
| func configure(weatherResponses: WeatherListResponse) { | |
| func configure(weatherListResponse: WeatherListResponse) { |
またはリストと詳細で同じように扱っているので、共通の型に Convert しても良さそうですね
ykws
left a comment
ykws
left a comment
There was a problem hiding this comment.
課題を対応してもらっているのを見て、内容が曖昧であることに気付かされたので、 Session13での対応内容を以下のように追記しています。
課題を対応してからで申し訳ないのですが、
研修の課題を気づかせてくれてこの PR に感謝しています。
|
|
||
| var model: WeatherFetcher! | ||
| private let viewController = R.storyboard.main.weatherViewController() | ||
| var model: WeatherListFetcher! |
There was a problem hiding this comment.
WeatherView 用のテストは変わらない想定でした。
追加するなら、 WeatherListView 用のテストを追加してみてください。
（リスト表示向けのテストを同じ PR で扱うのは重そうなので、オプション課題かなと思います）
There was a problem hiding this comment.
テストはこのPRでは扱わないことにします....
|
|
||
| import Foundation | ||
|
|
||
| final class Cache: NSCache<AnyObject, AnyObject> { |
There was a problem hiding this comment.
Cache を保存する責務を担うものを命名で表現したいです。
また Presenter に含めるべきかも再考をお願いします。
ただ、今の段階では、 List から Detail に値渡しでも十分だと思います。
There was a problem hiding this comment.
課題内容が変わっていたことに気づきませんでした...
行選択で、詳細画面へ遷移する
詳細画面は今まで作成していた天気予報の画面を呼び出し、APIからではなく、リストで選択した天気予報のデータを渡して表示できるようにしてみましょう
と書いてあるので、今実装してる３日先の天気予報の表示ではなく、単に値渡しして詳細とし表示するように修正します！
ykws
left a comment
ykws
left a comment
There was a problem hiding this comment.
リストでセルをタップして詳細から戻ってきてもセルがタップされた状態になっていました。
UITableView に標準でその機能が用意されているので是非対応方法を調べて試してみてください。
| func fetchWeather(areas: [String], date: Date) async -> Result<[WeatherListResponse], APIError> | ||
| } | ||
|
|
||
| class WeatherListFetcher: WeatherListFetchable { |
There was a problem hiding this comment.
WeatherFetcher は List と 1件取得まとめて実装してしまっても良さそうですね。
天気予報を取得する形式が違うだけなので、別 Model に分けるのは分割し過ぎに感じました。
また詳細画面で 1件取得の Reload ができなくなっているので、そもそも List だけで良いですかね。
2 participants
やったこと
課題へのリンク
API 仕様へのリンク
実行画面
懸念点