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

feat: Dropdown 及び FilterDropdown に onOpen/onClose オプションを追加する #4722

Merged
merged 9 commits into from
Jun 25, 2024

Conversation

s-sasaki-0529
Copy link
Contributor

@s-sasaki-0529 s-sasaki-0529 commented Jun 14, 2024

Related URL

N/A

Overview

Dropdown 及び FilterDropdown に onOpen/onClose オプションを追加し、ドロップダウンコンテンツの開閉をプロダクト側でフックできるようにします。

一度以下で提案した後、「あったら嬉しいけど無くてもよい」と結論付けて見送りましたが、やはりプロダクト側でドロップダウンコンテンツの開閉をフックに非同期処理を呼び出したいユースケースがあったため対応したいです。
https://kufuinc.slack.com/archives/C06KVP5PL23/p1711607266418609

調べてみると他のプロダクトでも潜在需要があるっぽい?
https://kufuinc.slack.com/archives/C05TN64KD1V/p1717059738348349

What I did

  • DropdownonOpen / onClose オプションを追加
  • FilterDropdownonOpen / onClose オプションを追加
  • ドロップダウンコンテンツの開閉状態が変化するたびに onOpen / onClose を発火する

@s-sasaki-0529 s-sasaki-0529 self-assigned this Jun 14, 2024
@s-sasaki-0529 s-sasaki-0529 marked this pull request as ready for review June 14, 2024 14:14
@s-sasaki-0529 s-sasaki-0529 requested a review from a team as a code owner June 14, 2024 14:14
@s-sasaki-0529 s-sasaki-0529 requested review from yt-ymmt and uknmr and removed request for a team June 14, 2024 14:14
Copy link
Contributor

@yt-ymmt yt-ymmt left a comment

Choose a reason for hiding this comment

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

一点だけ質問コメントしました!

Comment on lines 19 to 21
type Props = {
onToggle?: (active: boolean) => void
}
Copy link
Contributor

@yt-ymmt yt-ymmt Jun 17, 2024

Choose a reason for hiding this comment

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

onToggleの場合、開いたときと閉じたときで使用側で処理を分岐させると思うんですが、onOpen、onCloseとしてそれぞれで設定できるようにしたほうが良かったりしますかね?

現状onToggleに渡す関数内でほぼ必然的にif文が強制されるため、分けてあったほうが使いやすかったりもするかも?と思いコメントしております。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

言われるとその通りな気がしました!

元々僕のプロダクトでは onOpen さえあれば良いんですが、他のプロダクトでは onClose も欲しい気配がして雑に onToggle にまとめたんですが、各プロダクト側で必要に応じて分岐を書くよりは onOpen= と明示できたほうがわかりやすそうですねー。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちら修正対応しましたー!

@s-sasaki-0529 s-sasaki-0529 changed the title feat: Dropdown 及び FilterDropdown に onToggle オプションを追加する feat: Dropdown 及び FilterDropdown に onOpen/onClose オプションを追加する Jun 17, 2024
Copy link
Contributor

@yt-ymmt yt-ymmt left a comment

Choose a reason for hiding this comment

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

ありがとうございます!ばっちりです!LGTM

@uknmr uknmr merged commit acf3642 into master Jun 25, 2024
10 checks passed
@uknmr uknmr deleted the add-on-click-to-dropdown branch June 25, 2024 06:14
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.

3 participants