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

feat: ポモドーロタイマーの機能 #134

Merged
merged 16 commits into from
Jun 28, 2024

Conversation

Hirotaka-Hanai
Copy link
Contributor

@Hirotaka-Hanai Hirotaka-Hanai commented May 22, 2024

チケット

#92

実装内容

  • ポモドーロタイマーの機能の実装
    • スタート、一時停止、停止
    • 残り時間の表示
    • 残り時間0秒になった際の読み上げ・通知機能
    • 終了n分前の読み上げ・通知機能
    • ユニットテスト
      • contextを用いたrendererの処理にしたため、ユニットテストは行わない

懸念点

  • デスクトップ通知が表示されない不具合
    • npm startによる開発環境だけで発生している。npm run buildを行ってビルドしたアプリでは現状正常に動作している。
      • 一応、開発環境でも実装当初は動いていた。
    • 通知機能の概要
      • PomodoroTimerContext.tsxでタイマー機能を実装しており、その中の処理で通知機能を呼び出している。
      • 通知機能の実装はNotificationServiceImpl.tsにて、Notifications APIを用いて行っている。
        • ログを出力して確認した限りでは、Notifications APIを呼ぶところまではできているように見える。

@Hirotaka-Hanai Hirotaka-Hanai changed the title WIP: feat: ポモドーロタイマーの機能 feat: ポモドーロタイマーの機能 Jun 10, 2024
};
}, []);

// useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

ここはたぶん、コメントアウトを戻すのを忘れたのではないかと思うので確認してください

@msato-ok
Copy link
Contributor

@Hirotaka-Hanai
pullして実行してみたところエラーが出ていて、動作確認ができていないので、解消してもらって、再度、レビューとさせてください。

localhost-1719192426050.log

@Hirotaka-Hanai
Copy link
Contributor Author

@msato-ok
ローカルでエラーを再現できなかったので、エラー内容からそれっぽい修正を行い、ログを追加しました。
お手数ですが、これで動作確認を行っていただき、再度エラーがでるようでしたらもう一度ログを共有いただけないでしょうか?
よろしくお願いします。

@msato-ok
Copy link
Contributor

msato-ok commented Jun 24, 2024

@Hirotaka-Hanai

@msato-ok ローカルでエラーを再現できなかったので、エラー内容からそれっぽい修正を行い、ログを追加しました。 お手数ですが、これで動作確認を行っていただき、再度エラーがでるようでしたらもう一度ログを共有いただけないでしょうか? よろしくお願いします。

@msato-ok
Copy link
Contributor

msato-ok commented Jun 26, 2024

@Hirotaka-Hanai
設定画面のエラーは出なくなりました。DBを削除しなくても、エラーが出なくなったので、デフォルト値が無かったことが原因のようです。

ただし、まだ問題が2つ出ています。

  • タイムラインが表示されない
    image

    • この表示のままになってます。
    • 設定画面でエラーが出ていた、PR初期のときから、この状態でした
  • ポモドーロタイマーが動かない
    image

    • スタートボタンを押したけど、時間が進まない

@Hirotaka-Hanai
Copy link
Contributor Author

@msato-ok
設定項目が追加された場合でも、デフォルト値が反映されるように修正をいれました。
この状態でもう一度動作確認をお願いします。

@msato-ok
Copy link
Contributor

@Hirotaka-Hanai
最新を pull したら、2つの問題ともに、修正されていました。

get tableName(): string {
return 'userPreference.db';
}

async get(userId: string): Promise<UserPreference | undefined> {
return await this.dataSource.get(this.tableName, { userId: userId });
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

なるほど、デフォルト値の埋め込みは、確かに、こうしておかないとダメですね。

なぜ、こういう実装になっているか、クラス全体のドキュメントコメントのところに、説明を書いておいてもらってもいいですか?

Nit:
実装済コードがそうなのだけど、全般的にコメントが書けていないので、まとめて書くという作業も必要になりそうです。これは、別のタスクで対応します。

@msato-ok
Copy link
Contributor

@Hirotaka-Hanai
レビューしました。1つだけ、対応お願いします。
#134 (comment)

@Hirotaka-Hanai
Copy link
Contributor Author

@msato-ok
コメント追加しました。

@msato-ok
Copy link
Contributor

@Hirotaka-Hanai

LGTM

スカッシュして、マージしてください。
マージ済みとなったブランチは削除お願いします。

@Hirotaka-Hanai Hirotaka-Hanai merged commit 43ff9d1 into develop Jun 28, 2024
1 check passed
@Hirotaka-Hanai Hirotaka-Hanai deleted the feature/92/pomodoro-timer branch June 28, 2024 09:54
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.

None yet

2 participants