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

メニューを開くボタンをトグルにする #4137

Merged
merged 13 commits into from
Jan 3, 2024

Conversation

saitenntaisei
Copy link
Contributor

Copy link

github-actions bot commented Nov 2, 2023

@saitenntaisei saitenntaisei linked an issue Nov 2, 2023 that may be closed by this pull request
4 tasks
@saitenntaisei saitenntaisei self-assigned this Nov 2, 2023
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9e8f0d6) 86.35% compared to head (454fa36) 86.35%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4137   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files          66       66           
  Lines        4719     4719           
  Branches      564      564           
=======================================
  Hits         4075     4075           
  Misses        638      638           
  Partials        6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@saitenntaisei
Copy link
Contributor Author

3番目を直したら4番目が壊れた
4番目を直したら3番目が壊れた

@mehm8128
Copy link
Contributor

mehm8128 commented Nov 2, 2023

↑の翻訳
メッセージのコンテキストメニューで上手く動くようにしたらサイドバーのピン留めのメッセージで再度三点リーダーをクリックしたときにページがリロードされてしまうaタグのような動作でピン留めされているメッセージに移動する
ピン留めの方で上手く動くようにしたらメッセージコンテキストメニューが上手く動かなくなる

@mehm8128
Copy link
Contributor

mehm8128 commented Nov 3, 2023

stopPropagationしたらaタグで遷移しちゃうって話だったから、router-linkじゃなくてチャンネルリストでのチャンネル遷移みたいに↓のようにすれば一応動くようにはなりそう

const { openLink } = useOpenLink()
const { channelIdToLink } = useChannelPath()
const openChannel = (event: MouseEvent) => {
openLink(event, channelIdToLink(props.channel.id))
}

ただ、a11y的にはあんまりよくないので、できれば別の方法を考えたい

@mehm8128
Copy link
Contributor

mehm8128 commented Nov 3, 2023

そもそもaタグの中にボタンみたいなものが入っているのが間違ってるので、三点リーダーのボタンはrouter-linkの兄弟に置いてdisplay: absoluteで上に重ねるとかがいいかも

@mehm8128
Copy link
Contributor

mehm8128 commented Nov 3, 2023

大変そうなら一旦9点リーダーボタンとチャンネルのコンテキストメニューだけでPR作っちゃってもOKです!

@saitenntaisei
Copy link
Contributor Author

saitenntaisei commented Dec 6, 2023

できたはず acctiveelementが何かわからなかったそこのテストができてない
css わからなさ過ぎてrouterlinkのかかる場所を結局ずらした

@saitenntaisei
Copy link
Contributor Author

@mehm8128 rereqができなかったからmention:gomen:

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

だいぶ前で忘れちゃったんですけど、今の状態ってどこもtoggleできない(開いてる状態でクリックしても閉じてくれない)気がするんですけど僕の環境だけですか?

@mehm8128
Copy link
Contributor

acctiveelementが何かわからなかったそこのテストができてない

ってActivityElementのことですかね?これ↓だと思います
image

@saitenntaisei
Copy link
Contributor Author

マージが原因でなんかすべてが吹き飛んでた

@saitenntaisei
Copy link
Contributor Author

masterを取り込んだ瞬間にすべてが破壊される

@saitenntaisei
Copy link
Contributor Author

別に上書きも起きてないし分からず

@saitenntaisei
Copy link
Contributor Author

master マージする前

general.-.traQ.-.Google.Chrome.2023-12-28.11-44-01.mp4

@saitenntaisei
Copy link
Contributor Author

:kan:

@saitenntaisei saitenntaisei requested a review from mehm8128 January 1, 2024 05:13
@saitenntaisei
Copy link
Contributor Author

三点とか9点部分clickからmousedownにしても使用感ほとんど変わらないはずという考えで書いた

@mehm8128
Copy link
Contributor

mehm8128 commented Jan 1, 2024

mousedownにするのは個人的にはあんまりやりたくないですね🤔
現時点で思いついてるのは以下の2つ

  • ClickOutsideの対象からいい感じにボタンだけ外す
    • ClickOutsideのclose→それぞれのtoggleで再度表示されてしまっている状態なので、toggleが発火するようなボタンを押したときはClickOutsideのcloseが走らないようにすればいいはず
    • 例えばClickOutsideとボタンとポップアップがセットになってて、ボタンとポップアップをClickOutsideが囲ってるコンポーネントを作るとか?
  • closeする関数をsetTimeoutの秒数0で囲む
    • setTimeout 0とかで調べれば色々出てくると思うけど、toggleした後にcloseするようにできるので、表示された状態ならclose→closeになって動作自体は問題なくなる

@saitenntaisei
Copy link
Contributor Author

setTimeout 0が楽そうだったのでそれで解決

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

importの順序変わって余計な差分出てそうなので直してほしいです(機能の差分が分かりづらくてレビューしづらいので)

@saitenntaisei
Copy link
Contributor Author

これなんで発生してるのかわからんくて困ってる

@mehm8128
Copy link
Contributor

mehm8128 commented Jan 2, 2024

保存時に勝手に入れ替わるなら拡張機能のせいか、手元のglobalなprettier or eslintの設定が変に効いちゃってるとかだと思います
保存時に入れ替わらないなら分からないので、とりあえず手動で戻しておいてください

@saitenntaisei saitenntaisei requested a review from mehm8128 January 2, 2024 08:01
@saitenntaisei
Copy link
Contributor Author

saitenntaisei commented Jan 3, 2024

@mehm8128 .containerと.iconのやつ直しました
reviewお願いします

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

よさそうだと思ったのですが、スマホでサイドバーを開いたときに暗くなってる部分をタップしてもサイドバーが閉じなくなってしまってますね:eyes_komatta:(画像を赤く囲った部分)
image

@saitenntaisei
Copy link
Contributor Author

saitenntaisei commented Jan 3, 2024

それ自分の環境(pixel4a)だともとからできないが

@mehm8128
Copy link
Contributor

mehm8128 commented Jan 3, 2024

あれ、ほんとだ
別でissue立てておきます

@saitenntaisei
Copy link
Contributor Author

saitenntaisei commented Jan 3, 2024

re-req投げれば良い感じ?

Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

じゃあ問題なさそうです

@saitenntaisei saitenntaisei merged commit 7133947 into master Jan 3, 2024
11 checks passed
@saitenntaisei saitenntaisei deleted the saiten/issue3773 branch January 3, 2024 05:42
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.

メニューを開くボタンをトグルにする
2 participants