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

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

Merged
merged 9 commits into from
Jun 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
95803ef
feat: Dropdown 及び FilterDropdown に onOpen オプションを追加
s-sasaki-0529 Jun 14, 2024
7126666
fix: onOpen でなく onToggle にする
s-sasaki-0529 Jun 14, 2024
03f581a
fix: 不要なコードを削除
s-sasaki-0529 Jun 14, 2024
f1a479b
fix: useEffect を分離する
s-sasaki-0529 Jun 14, 2024
1905526
fix: 不要な差分を戻す
s-sasaki-0529 Jun 14, 2024
d7245a3
fix: マウント時に発火しないようにする
s-sasaki-0529 Jun 14, 2024
44cc787
fix: active の変更でのみ発火させる
s-sasaki-0529 Jun 14, 2024
c823cc5
fix: eslint 違反を無視する
s-sasaki-0529 Jun 14, 2024
ec31a5c
fix: onToggle を onOpen / onClose に分割
s-sasaki-0529 Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion packages/smarthr-ui/src/components/Dropdown/Dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ import { usePortal } from '../../hooks/usePortal'

import { Rect, getFirstTabbable, isEventFromChild } from './dropdownHelper'

type Props = {
onToggle?: (active: boolean) => void
}
Copy link
Contributor

@yt-ymmt yt-ymmt Jun 17, 2024 •
edited
Loading

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.

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


type DropdownContextType = {
active: boolean
triggerRect: Rect
Expand Down Expand Up @@ -44,7 +48,7 @@ export const DropdownContext = createContext<DropdownContextType>({
contentId: '',
})

export const Dropdown: FC<PropsWithChildren> = ({ children }) => {
export const Dropdown: FC<PropsWithChildren<Props>> = ({ onToggle, children }) => {
const [active, setActive] = useState(false)
const [triggerRect, setTriggerRect] = useState<Rect>(initialRect)

Expand Down Expand Up @@ -82,6 +86,14 @@ export const Dropdown: FC<PropsWithChildren> = ({ children }) => {
},
[active, createPortal, isPortalRootMounted],
)

useEffect(() => {
if (isPortalRootMounted() && onToggle) {
onToggle(active)
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [active])

// set the displayName explicit for DevTools
DropdownContentRoot.displayName = 'DropdownContentRoot'

Expand Down
4 changes: 3 additions & 1 deletion packages/smarthr-ui/src/components/Dropdown/FilterDropdown/FilterDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type Props = {
onApply: React.MouseEventHandler<HTMLButtonElement>
onCancel?: React.MouseEventHandler<HTMLButtonElement>
onReset?: React.MouseEventHandler<HTMLButtonElement>
onToggle?: (active: boolean) => void
children: ReactNode
hasStatusText?: boolean
decorators?: DecoratorsType<
Expand Down Expand Up @@ -73,6 +74,7 @@ export const FilterDropdown: FC<Props & ElementProps> = ({
onApply,
onCancel,
onReset,
onToggle,
children,
hasStatusText,
decorators,
Expand Down Expand Up @@ -140,7 +142,7 @@ export const FilterDropdown: FC<Props & ElementProps> = ({
}, [isFiltered, triggerSize])

return (
<Dropdown>
<Dropdown onToggle={onToggle}>
<DropdownTrigger>
<Button
{...props}
Expand Down