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

imprv: Show unsaved warning when comment not posted #7603

Conversation

arafubeatbox
Copy link
Contributor

review task

https://redmine.weseek.co.jp/issues/121003

動作イメージ

コメント編集中も同様
Apr-24-2023 21-29-43

Comment on lines 240 to 243
const onChangeHandler = useCallback((newValue: string, isClean: boolean) => {
setComment(newValue);
mutateIsEnabledUnsavedWarning(!isClean);
}, [mutateIsEnabledUnsavedWarning]);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • post しても引き続き browser warning がでます。ご確認お願いします。
    comment post 後に渡る値を見てみると newValue は ですが, isClean は false でわたってくるようです。

  • Reply と Add Comment の CommentEditor が同時に呼ばれる場合があるのでどちらかに not posted な値があるときは browser warning を出すようにしてください。(両方に入力してから片方 post したら warning されない気がする。)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

対応しました。

@arafubeatbox arafubeatbox temporarily deployed to VRT April 25, 2023 14:25 — with GitHub Actions Inactive
Comment on lines +110 to +122
const initializeEditor = useCallback(async() => {
setComment('');
setActiveTab('comment_editor');
setError(undefined);
initializeSlackEnabled();
// reset value
if (editorRef.current == null) { return }
editorRef.current.setValue('');
}, [initializeSlackEnabled]);
const editingCommentsNum = await decrementEditingCommentsNum();
if (editingCommentsNum === 0) {
mutateIsEnabledUnsavedWarning(false); // must be after clearing comment or else onChange will override bool
}
}, [initializeSlackEnabled, mutateIsEnabledUnsavedWarning, decrementEditingCommentsNum]);
Copy link
Contributor

@jam411 jam411 Apr 27, 2023

Choose a reason for hiding this comment

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

cancelButtonClickedHandler() 内でもこの initializeEditor() 呼ぶようにしてください。
(なぜこれまで呼ばれてなかったんだろう。呼んでもいいはず。デグレがないか確認していただけるとありがたいです。)

@arafubeatbox arafubeatbox temporarily deployed to VRT April 28, 2023 01:10 — with GitHub Actions Inactive
@arafubeatbox arafubeatbox temporarily deployed to VRT April 28, 2023 01:29 — with GitHub Actions Inactive

return {
...swrResponse,
increment: () => swrResponse.mutate(swrResponse.data ? swrResponse.data + 1 : 1),
Copy link
Member

Choose a reason for hiding this comment

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

細かいけど、

コーディングガイド的には三項演算子の条件に使うなら != null と明示したい。
完結に書くなら (swrResponse.data ?? 0) + 1 でよさそう。

if (swrResponse.data != null && swrResponse.data > 0) {
return swrResponse.mutate(swrResponse.data - 1);
}
return swrResponse.mutate(0);
Copy link
Member

Choose a reason for hiding this comment

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

こちらも完結に、

const newValue = (swrResponse.data ?? 0) - 1);
mutate(Math.max(0, newValue))`;

@arafubeatbox arafubeatbox temporarily deployed to VRT April 28, 2023 10:42 — with GitHub Actions Inactive
@reg-suit
Copy link

reg-suit bot commented Apr 28, 2023

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴
⚪⚪
⚫⚫
🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@jam411 jam411 merged commit 941c546 into master Apr 28, 2023
@jam411 jam411 deleted the imprv/120709-121002-show-browser-alert-when-comment-is-not-posted branch April 28, 2023 11:25
@github-actions github-actions bot mentioned this pull request Apr 28, 2023
@yuki-takei yuki-takei changed the title imprv: show alert when comment not posted imprv: Show unsaved warning when comment not posted May 11, 2023
@yuki-takei yuki-takei mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants