-
Notifications
You must be signed in to change notification settings - Fork 113
Conversation
[提案] PR のタイトルをPRの内容を説明したものに変更してみませんか? |
ありがとうございます。変更しました。(まだ慣れてません) |
|
||
namespace Covid19Radar.ViewModels | ||
{ | ||
public class DebugPageViewModel : ViewModelBase |
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.
[提案] Debug Page 関連は他の画面と少し毛色が違うのでディレクトリ切ってそこに入れませんか?
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.
このページだけのディレクトリを作るのは避けたいのですが、既存のもの(HelpPage, Settings..)だとどれが良いでしょうか?現状のページ配置も、それほど意味を見出せないのですが・・
・・・と、私は悩んで、どれでもいいかとなってしまっています。
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.
DebugはDebugというだけでドメインが違うので単体でフォルダを切る価値があると思いました。
下手にフォルダの中に入れるのではなく、Viewの直下においても良いかもですね。
現状のページ配置も、それほど意味を見出せないのですが・・.
それな...
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.
もし開発チームに意向があればこちらに届けば反映する、という方針にして、ここは見えるようにおいておきます。
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.
これは要検討ですね。
itemGroupのような仕掛けでReleaseビルドに含めないようにするのなら、1つのディレクトリ(たとえばDebugPage
など)に集約されていることに意味はあると思います。
一方で、今の名前空間がView
やViewModel
といった分け方なので、そこにいきなり機能単位のDebugPage
が来るのは違和感が拭えません。
となると、View/DebugPage
とViewModel/DebugPage
のようにするのが納得感がある反面、ディレクトリが深くなっただけで1つのファイルしか保存していないなら今のままでいいじゃないかという…
まずはここは課題として残しておき、機能面やその他の場所を見ていきましょう。どこに配置するかは頃合いを見て決めると言うことで。
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.
参考までに、元々あったデバッグページは設定ページと同じディレクトリに置いてありました。
https://github.com/cocoa-mhlw/cocoa/tree/4c4aca39b02dcb1bd89569674b65b0ef6a357bae/Covid19Radar/Covid19Radar/Views/Settings
同じ場所に置くと GitHub の History ボタンから履歴が見やすくなると思います。
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.
同じ場所に置くと GitHub の History ボタンから履歴が見やすくなると思います。
少なくとも HomePage/ よりは、まともな根拠と思いますので、Settings/ に変えたいですね。ました。
80128f5
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
x:Class="Covid19Radar.Views.DebugPage" | ||
xmlns="http://xamarin.com/schemas/2014/forms" | ||
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
xmlns:ffimageloading="clr-namespace:FFImageLoading.Forms;assembly=FFImageLoading.Forms" |
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.
必要ですか?
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.
安全のための HomePage.xaml からの流用です。とりあえず,このまま同じにしておきますので、一斉除去する機会に掃除してください。
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.
ここはVisual Studioさんも指摘している箇所なので、適宜(次に何か手を加えるタイミングででも)消しておいてもらえるとうれしいです!(見落としが発生するかもしれないので、いったんUnresolveにしておきますね。
ちなみに、開発に参加している人たちがお互いのコードを確認して、気がついたことを指摘する行程をコードレビューと言います。オープンソースでは一般的で、コードをよりよいものにすることを目的としていて、コードの欠点を見つけることは目的としていないのでご安心ください。
コードレビューはコミッター(コードの取り込みを決定する権限のある人)だけでなく、コントリビューター相互で行われる場合があります。
大きいPull Requestだとコードレビューで出てくる指摘も多くなりますし、指摘と修正のやりとりにかかる期間も長くなります。コミッターからの指摘に応じてすぐに直してもらっても、次に同じPull Requestを見るまで期間が空いてしまったりします。なので、さまざまなコントリビューターにレビューに参加してもらえているのは本当にありがたい限りです。
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.
こちら初心者(GitHubも)でして、お手数ですが Resolve / Unresolve も適宜お願いいたします。
この部分については、HomePage からの流用であっても xmlns:ffimageloading は不要ですね。(今のDebugPage は画像を使っていないので)
そういうご指摘だったということを今理解しました。なので一つ上の私の解答は、的外れです。
おかげさまで今回、驚くことや学ぶことは多く、皆さんに感謝しています。(迅速なレビューとGitHub PR上のコミット、オンラインの実行テストなど)
…el.cs Co-authored-by: kuu(Fumiya Kume) <kuxu2525@yahoo.co.jp>
…el.cs Co-authored-by: kuu(Fumiya Kume) <kuxu2525@yahoo.co.jp>
…el.cs Co-authored-by: kuu(Fumiya Kume) <kuxu2525@yahoo.co.jp>
…el.cs Co-authored-by: kuu(Fumiya Kume) <kuxu2525@yahoo.co.jp>
ありがとうございました。一応エミューレータの再起動を確認してプッシュしました。 |
Covid19Radar/Covid19Radar/ViewModels/HomePage/DebugPageViewModel.cs
Outdated
Show resolved
Hide resolved
…el.cs Co-authored-by: kuu(Fumiya Kume) <kuxu2525@yahoo.co.jp>
i-maruyama#1 にて |
このようにするのですね。ありがとうございます。取り込みます。 |
info() to Info() c.f.: cocoa-mhlw#178 (comment)
返信が遅れて申し訳ありません。 |
`Release` ビルド時にデバッグページを除外して容量を削減する
確認しました。Covid19Radar.csproj へのPRをマージして、現在のディレクトリ Settings に変更しました。 |
確認しました所、下記の2点が気になりました。
|
This reverts commit 857bc1c.
ありがとうございました。1ファイルだけステージングするのを忘れて、
これもご確認ありがとうございました。( #182 と似た感じでしょうか?) 再度まとめると、もともと 8fbbc10 では、 これに、 @Takym さんから PR された Covid19Radar.csproj への変更をマージし、 |
var str = new[] { "Build: " + os, "Ver: " + AppSettings.Instance.AppVersion, | ||
"Region: " + string.Join(",", AppSettings.Instance.SupportedRegions), "CdnUrl: " + AppSettings.Instance.CdnUrlBase, | ||
"ApiUrl: " + AppSettings.Instance.ApiUrlBase, "Agree: " + agree, "StartDate: " + userDataService.GetStartDate().ToLocalTime().ToString("F"), | ||
"DaysOfUse: " + userDataService.GetDaysOfUse(), "ExposureCount: " + exposureNotificationService.GetExposureCountToDisplay(), |
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.
v1.2.4 に対応するため、GetExposureCountToDisplayにしました。
#if DEBUG | ||
MenuItems.Add(new MainMenuModel() | ||
{ | ||
Icon = "\uf013", |
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.
本質的ではないのですが、虫Icon があるようですね。そちらにしたくなってきました。
var exposureNotificationStatus = await Xamarin.ExposureNotifications.ExposureNotification.IsEnabledAsync(); | ||
var exposureNotificationMessage = await exposureNotificationService.UpdateStatusMessageAsync(); | ||
// ../../settings.json | ||
var str = new[] { "Build: " + os, "Ver: " + AppSettings.Instance.AppVersion, |
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.
setting.json から設定される AppSettings.Instance.AppVersion,Write は、意味のない情報のようで、ログや、バージョンチェックで使われるのは, EssentialsService.AppVersion のようですね。
何を表示させるか?は、もう少し議論が必要だと思います。
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.
表示するのはEssentialsService.AppVersion
にしましょう。
@i-maruyama Pull Requestの向き先を |
お願いします。 |
向き先変えました! お手数ですがコンフリクトの解消お願いします! |
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.
#if DEBUG | ||
MenuItems.Add(new MainMenuModel() | ||
{ | ||
Icon = "\uf013", | ||
PageName = nameof(DebugPage), | ||
Title = "debug", | ||
IconColor = "#019AE8", | ||
TextColor = "#000" | ||
}); | ||
#endif |
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.
デバッグ画面のカラムは一番下に置きましょう(あまり主張したくないので)。
{ | ||
Icon = "\uf013", | ||
PageName = nameof(DebugPage), | ||
Title = "debug", |
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.
Title = "debug", | |
Title = "Debug", |
var termsUpdateInfo = await termsUpdateService.GetTermsUpdateInfo(); | ||
if (termsUpdateService.IsReAgree(TermsType.TermsOfService, termsUpdateInfo)) | ||
{ | ||
agree += "-TermsOfService"; | ||
} | ||
else if (termsUpdateService.IsReAgree(TermsType.PrivacyPolicy, termsUpdateInfo)) | ||
{ | ||
agree += "-PrivacyPolicy"; | ||
} |
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.
ここはtermsUpdateInfo
に含まれているTermsOfService
とPrivacyPolicy
の最新の値(更新日付?)を表示する方が良いかなと思いました。
ここは「利用許諾」と「プライバシーポリシー」が更新されている経路だと理解しています。
一般的にデバッグ画面に到達したときにはすでプライバシーポリシーの再同意が行われている状態になるので、-TermsOfService
と-PrivacyPolicy
がつくことは極めて稀(実行中に規約類が更新されるタイミング?)なので、前述通り、TermsOfService
とPrivacyPolicy
の最新の値(更新日付?)を表示する方が良いかと思います。
string agree; | ||
if (termsUpdateService.IsAllAgreed()) | ||
{ | ||
agree = "exists";// (mainly) navigate from SplashPage to HomePage |
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.
agree = "exists";// (mainly) navigate from SplashPage to HomePage | |
agree = "All agreed"; // (mainly) navigate from SplashPage to HomePage |
} | ||
else | ||
{ | ||
agree = "not exists"; // navigate from SplashPage to TutorialPage1 |
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.
デバッグ画面に到達するには初期設定が必要と理解しているので、ここの経路に到達することはないかと思いますが、仮にこの経路があるとすれば、
agree = "not exists"; // navigate from SplashPage to TutorialPage1 | |
agree = "Not agreed"; // navigate from SplashPage to TutorialPage1 |
string os; | ||
switch (Device.RuntimePlatform) | ||
{ | ||
case Device.Android: | ||
os = "Android"; | ||
break; | ||
case Device.iOS: | ||
os = "iOS"; | ||
break; | ||
default: | ||
os = "unknown"; | ||
break; | ||
} |
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.
Device.RuntimePlatform
をそのまま使えば、OSプラットフォームの文字列をとることができます。Windowsとかも入ってしまいますが、そもそもビルドできないので考慮しなくて良いと思います。
string os; | |
switch (Device.RuntimePlatform) | |
{ | |
case Device.Android: | |
os = "Android"; | |
break; | |
case Device.iOS: | |
os = "iOS"; | |
break; | |
default: | |
os = "unknown"; | |
break; | |
} | |
string os = Device.RuntimePlatform; |
var exposureNotificationStatus = await Xamarin.ExposureNotifications.ExposureNotification.IsEnabledAsync(); | ||
var exposureNotificationMessage = await exposureNotificationService.UpdateStatusMessageAsync(); | ||
// ../../settings.json | ||
var str = new[] { "Build: " + os, "Ver: " + AppSettings.Instance.AppVersion, |
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.
文字列連結は + ではなく、$
を付けた変数を展開する記法を使ってください。
$"Build: {os}, Version: {EssentialsService.AppVersion}"
のようになります。
あと、Ver
はVersion
にするなど、短い単語であれば可能な限り略称を使わないようにしてください。
<Button | ||
Command="{prism:NavigateTo 'DebugPage'}" | ||
Style="{StaticResource DefaultButton}" | ||
Text="DebugPage" /> |
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.
DebugPageからDebugPageを開けるのはあまり意味がないと思うので、削除してください。
<Button | ||
Command="{prism:NavigateTo 'NavigationPage'}" | ||
Style="{StaticResource DefaultButton}" | ||
Text="NavigationPage" /> |
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.
ドロワーがある空白のページが開くだけなので削除して良いと思います。
<Button | ||
Command="{prism:NavigateTo 'MenuPage'}" | ||
Style="{StaticResource DefaultButton}" | ||
Text="MenuPage" /> |
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.
タップしても何も起きないので、削除して良いと思います。
@i-maruyama |
申し訳ありません。こちらのVisualStudio にエラーがでる状態が解消できずにおりますので、 頂いているレビューコメントも、おっしゃっている方向で変更することに同意いたします。 |
@i-maruyama コンフリクト解消のためのPull Requestを出しました。 このPRをマージいただいた後、このPRを |
Issue#160
Issue 番号 / Issue ID
目的 / Purpose
破壊的変更をもたらしますか / Does this introduce a breaking change?
Pull Request の種類 / Pull Request type
検証方法 / How to test
プロプロセッサによって Release ビルドでは、メニューに表示されません。
コードの入手 / Get the code
コードの検証 / Test the code
確認事項 / What to check
その他 / Other information
スクリーンショットを #160 に貼っておきます。
Internal IDs: