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.

Sign up for GitHub

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

Jump to bottom

モーダルを開いてるときにescキーを押したら閉じられるように #4121

Merged
merged 4 commits into from
Jan 6, 2024
Merged

モーダルを開いてるときにescキーを押したら閉じられるように #4121

merged 4 commits into from
Jan 6, 2024

Conversation

mehm8128
Copy link
Contributor

@mehm8128 mehm8128 commented Nov 1, 2023 •
edited
Loading

close #745

@mehm8128 mehm8128 self-assigned this Nov 1, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 1, 2023

Preview (prod) → https://4121-prod.traq-preview.trapti.tech/
Preview (dev) → https://4121-dev.traq-preview.trapti.tech/

@codecov Codecov
Copy link

codecov bot commented Nov 1, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@mehm8128 mehm8128 changed the title ファイルモーダル開いてるときにescキーを押したら閉じられるように モーダルを開いてるときにescキーを押したら閉じられるように Nov 8, 2023
ras0q
ras0q requested changes Nov 13, 2023
View reviewed changes
Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

コードの修正案を書きました
これってどこのモーダルを指してますか？

src/store/ui/modal.ts Outdated
Comment on lines 151 to 157
const onKeyDown = (e: KeyboardEvent) => {
if (e.key === 'Escape' && shouldShowModal.value) {
popModal()
}
}

window.addEventListener('keydown', onKeyDown)
Copy link
Member

Choose a reason for hiding this comment

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

ざっとaddEventListenerのreferencesを見た感じ関数が再利用されてなければ無名関数を使っているところが多そうです
個人的にもそっちの方が好みなので余裕があれば修正してほしいです:pray:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かにそうですね、直します

@ras0q
Copy link
Member

ras0q commented Nov 13, 2023

optionalなのにchange requestedしちゃた

@mehm8128
Copy link
Contributor Author

影響あるのはここにあるモーダル全部だと思ってます

traQ_S-UI/src/store/ui/modal/states.ts

Lines 11 to 26 in e4a04d4

type ModalStateType =
| 'user'
| 'group'
| 'notification'
| 'file'
| 'tag'
| 'group'
| 'channel-create'
| 'qrcode'
| 'clip-create'
| 'clip-folder-create'
| 'channel-manage'
| 'group-create'
| 'group-member-edit'
| 'group-admin-add'
| 'group-member-add'

@SSlime-s
Copy link
Member

通知メンバーいじるところの入力欄とかの一部モーダルで、Esc で Clear する挙動になっているので、そこと競合してそうです (競合というか Clear を期待して Esc を押すと入力が消えて萎える)
すべて試してはないけど FilterInput を使っている部分 少なくともメンバー選ぶ系 (グループ周りとか) は同じようになってそうです

  1. esc 入力時に Clear している部分調査してそこに .stop (stopPropagation)つける
  2. modal 側で esc 時に activeElement が input じゃないときにのみ動作するようにする (こっちだと Clear するわけではない input でも閉じなくなる)

のどっちかをするとよさそう

@mehm8128
Copy link
Contributor Author

ありがとうございます！見落としてました
対処法としては前者にしようと思うのですが、

  • 1文字以上入れてるときだけstopして0文字のときはモーダル閉じるようにする
  • 文字数関係なくフォーカス時にescを押したときはクエリのリセットだけされるようにする

どちらがいいと思いますか？

@SSlime-s
Copy link
Member

特に強い主張はないんですが、難しいことを言うと

  • 通知モーダル -> 検索してトグルするだけだから、空なら閉じてほしい
  • グループ系モーダル -> 検索して選択してまとめて追加するので、空でも閉じないでほしい

って感じです
個人的にはどうせ前者もキーボード操作だけで完結させられないので、空でも閉じなくていいんじゃないかと思います

@mehm8128
Copy link
Contributor Author

なるほどです、確かにそうかもです

じゃあ統一して空でも閉じない方針にします:+1:

@mehm8128 mehm8128 requested a review from ras0q November 13, 2023 12:01
ras0q
ras0q approved these changes Jan 6, 2024
View reviewed changes
Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

動作確認しました
コードもいいと思います

@mehm8128 mehm8128 merged commit ca5871a into master Jan 6, 2024
11 checks passed
@mehm8128 mehm8128 deleted the feat/close_file_modal_on_esc branch January 6, 2024 08:53
@anko9801 anko9801 mentioned this pull request Feb 17, 2024
Open
@cp-20 cp-20 mentioned this pull request Jun 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ras0q ras0q ras0q approved these changes

@zer0-star zer0-star Awaiting requested review from zer0-star

@nokhnaton nokhnaton Awaiting requested review from nokhnaton

Assignees

@mehm8128 mehm8128

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

各種モーダルをescでとじたい
3 participants
@mehm8128 @ras0q @SSlime-s