-
Notifications
You must be signed in to change notification settings - Fork 0
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
通知が1回のみ表示されるようにした #107
通知が1回のみ表示されるようにした #107
Conversation
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.
レビューしました。
質問に回答してくれたらマージしてオーケーよ
LGTMです!!
@@ -28,7 +28,7 @@ export const loader = async ({ request }: LoaderFunctionArgs) => { | |||
|
|||
// 未ログインの場合 | |||
// エラーメッセージを取得 | |||
const data = { error: session.get('loginError') }; | |||
const data = { error: session.get('error') }; |
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.
sessionは一律errorにしたんか
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.
異なるrouteが同じflashを読み出さないように、flashの種類を分けてたんだけど、エラーメッセージの増加に伴って管理コストが増大するので、1つに絞ることにした
frontend/app/routes/auth.login.tsx
Outdated
session.set('userId', response.data.id.toString()); | ||
session.set('sessionToken', response.data.sessionToken!); | ||
// FIXME: homeのloaderで読み出してもCookieが削除されない |
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.
そのコメントは消しておかねば
if (error) { | ||
errorNotification(error); | ||
} | ||
}, [error]); |
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.
結局このやり方にしたんか
このやり方やと、同じエラーが連発した時に通知が一回しか出ない問題があったけど解決したん?
flashで入れてるから一回undefinedに変換されて良い感じにuseEffectが呼ばれるようになるとか
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.
ログイン画面ではerrorではなくnavigation.stateを依存関係にしているので、エラーが発生した回数だけ通知が表示される
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.
他の画面ではおなじerrorは連続で呼ばれないだろってイメージ?
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.
なるほど、ログイン画面以外で同じエラーが連続して発生したときにちゃんと通知がでるんか?ってことだね
通知がでてから一度loaderが走れば、errorがundefinedになるので、同じエラーでも通知が出てくれると思う
手元の環境で試してみる。エラーは再現が難しいので、本の削除を2回繰り返したときに、成功の通知が2回とも出るか?を確かめてみる。
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.
2回は出なかった
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.
useRevalidator
を使ってsuccessとerrorの値を再検証することで解決した
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.
ならマージして良いよ!
やったこと
タイトル参照
確認した方法
目視で確認
スクリーンショット
2024-10-29.23.32.41.mov
自動生成したコード
なし