Skip to content
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

Session 2: API #2

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Session 2: API #2

merged 8 commits into from
Feb 21, 2024

Conversation

kantacky
Copy link
Owner

Session 2: API

Updates

  • 天気の画像を追加
  • 天気の列挙型 (晴れ・くもり・雨) を作成
  • ForecastViewで使うViewModelを作成
  • ViewModelをForecastViewで使うように変更
  • 天気の画像に色をつけるように返却するViewを変更

@kantacky kantacky requested review from a team February 16, 2024 06:17
@kiy00
Copy link

kiy00 commented Feb 16, 2024

mainに対してのPRではなく、Session1のブランチに対してマージするPRにしてみましょうか🤔
そうすることで、このPRにSession1の差分が入らず、レビューしやすい状態となります。
やり方は右上のEditボタンを押して、PRタイトルの下あたりからbase: mainを変更するとできます!

現状のPRは

%%{init: { 'gitGraph': { 'showCommitLabel': false } } }%%
gitGraph
  commit
  branch session1
  commit
  commit
  branch session2
  commit
  commit
  checkout main
  merge session2
Loading

↑のように、mainsession2の差分を見るPRになっているから、マージ先をsession1にしてやると

%%{init: { 'gitGraph': { 'showCommitLabel': false }} }%%
gitGraph
  commit
  branch session1
  commit
  commit
  branch session2
  commit
  commit
  checkout session1
  merge session2
Loading

こうなって、session1session2の差分だけを見るPRになる。

また、この設定にすると、Session1がマージされた時に自動的にこのPRがmainに向くようになります。

@kantacky
Copy link
Owner Author

@kiy00
ありがとうございます!

また、この設定にすると、Session1がマージされた時に自動的にこのPRがmainに向くようになります。

なるほど!これなら確かに、その方がいいですね!

@kantacky kantacky changed the base branch from main to session/1 February 16, 2024 09:35
Copy link

@ykws ykws left a comment

Choose a reason for hiding this comment

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

session1 の Git の履歴が繋がっていないようなので rebase か merge が必要です

スクリーンショット 2024-02-19 17 14 00

https://github.com/yumemi-training/ios-training-oikawa/network

YumemiTraining/WeatherCondition.swift Outdated Show resolved Hide resolved
import Foundation
import YumemiWeather

@MainActor
Copy link

Choose a reason for hiding this comment

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

ask-badge

@MainActor にしている意図はありますか?

Copy link
Owner Author

Choose a reason for hiding this comment

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

メインスレッドから利用されるので明示したい意図がありました!

Copy link

Choose a reason for hiding this comment

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

この処理をメインスレッドで利用したい動機は何になりますか?

Copy link
Owner Author

@kantacky kantacky Feb 20, 2024

Choose a reason for hiding this comment

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

@Publishedプロパティがメインスレッドでのみ変更されるようにしたいからです

Copy link
Owner Author

Choose a reason for hiding this comment

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

#7 で大きくViewModelを更新しました。

Copy link

Choose a reason for hiding this comment

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

スレッドを意識しているのは Good です!
質問の意図としては、今の実装だとバックグラウンドで処理されないのでそこを認識した上なのか気になっての質問でした。

@kantacky
Copy link
Owner Author

session1 の Git の履歴が繋がっていないようなので rebase か merge が必要です

スクリーンショット 2024-02-19 17 14 00 https://github.com/yumemi-training/ios-training-oikawa/network

session/1がmainにマージされた後にするつもりでいました...

Copy link

@ykws ykws left a comment

Choose a reason for hiding this comment

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

LGTM 👍

.resizable()
.foregroundStyle(.blue)
}
}
Copy link

Choose a reason for hiding this comment

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

next-badge

WeatherCondition の位置付けによっては、 SwiftUI に依存しない方が良いです

Button {
// TODO: Reload Action
// Reload Action
Copy link

Choose a reason for hiding this comment

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

imo-badge

冗長なコメントに感じました

Suggested change
// Reload Action
// Reload Action

weatherCondition.image
.scaledToFit()
} else {
Color.gray
Copy link

Choose a reason for hiding this comment

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

imo-badge

エラーを握り潰してしまっている箇所なので TODO コメントなどあると良いです

@kantacky kantacky merged commit e85d41c into session/1 Feb 21, 2024
@novr novr mentioned this pull request Feb 22, 2024
@kantacky kantacky deleted the session/2 branch February 22, 2024 02:21
@kantacky kantacky restored the session/2 branch February 22, 2024 02:22
@kantacky kantacky deleted the session/2 branch February 22, 2024 02:22
@kantacky kantacky restored the session/2 branch February 22, 2024 02:22
@kantacky kantacky deleted the session/2 branch February 22, 2024 02:24
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.

3 participants