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

fix: アプリケーション登録画面で、登録処理中にボタンを押せないように #935

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

cp-20
Copy link
Contributor

@cp-20 cp-20 commented Jul 8, 2024

なぜやるか

close #911

やったこと

アプリケーション登録画面で、登録処理中にボタンを押せないように

やらなかったこと

特になし

コメント

他のところと違う実装方法なんですが、分からなかったのでとりあえずこういう形で実装してみました
もっと良い実装方法があれば教えて欲しいです

@cp-20 cp-20 requested a review from eyemono-moe July 8, 2024 07:19
Copy link

github-actions bot commented Jul 8, 2024

Preview (prod backend + PR dashboard) → https://935.ns-preview.trapti.tech/

Copy link
Contributor

@eyemono-moe eyemono-moe left a comment

Choose a reason for hiding this comment

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

修正感謝です🙏
修正方針これでいいと思います👍
try/finallyのやつだけ確認お願いします 🙏

Comment on lines 443 to 445
setIsSubmitting(true)
await props.submit()
setIsSubmitting(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

これってprops.submit()がerrorをthrowしたときってsetIsSubmitting(false)実行されましたっけ...?(されない気がする)

await props.submit()tryで囲んで、setIsSubmitting(false)finallyで囲んだ方が良いかもです :tanonda:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そう思ったんですが、ここのsubmit関数が既にtry-catchされてたのでthrowすることが確実にないなと思ってそのまま書いちゃいました

他の場所で使われる (throwする可能性のあるsubmitが渡される) ことを考慮して書いておいたほうが良いですかね?

Copy link
Contributor

Choose a reason for hiding this comment

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

あ~submit関数の中までちゃんと見れてませんでした、だとしたら書いても書かなくてもいいかもですね...(好みの問題かも)

僕だったら

他の場所で使われる (throwする可能性のあるsubmitが渡される) ことを考慮して書いておいたほうが良いですかね?

の気持ちでfinally書いちゃうと思いますがcp-20さんに任せます

どちらにせよこのままでもちゃんとうごくのでapproveしちゃいます

@@ -589,7 +593,6 @@ export default () => {
setCurrentStep(formStep.repository)
// 選択していたリポジトリをリセットする
setParam({ repositoryID: undefined })
mutateRepo(undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

確かにこの部分いらなかったですね、削除感謝です 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(何もしてない気がするので、Linterが消した気がする……w)

@cp-20 cp-20 enabled auto-merge July 8, 2024 15:21
@cp-20 cp-20 merged commit 16ede67 into main Jul 15, 2024
17 checks passed
@cp-20 cp-20 deleted the fix/application-creation-submitting-disabled branch July 15, 2024 05:35
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.

Submit系のボタンを押したらdisabledにする
2 participants