Skip to content

fix(ModelessDialog): title propsがReactNodeを受け取れるように修正 #5907

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

Open
Qs-F wants to merge 5 commits into
base: master
Choose a base branch
from
+18 −9
Open

fix(ModelessDialog): title propsがReactNodeを受け取れるように修正 #5907

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64da431
feat: add PropsWithHTMLAttributes type
Qs-F Oct 22, 2025
8a505c9
fix(ModelessDialog): title propsがReactNodeを受け取れるように修正
Qs-F Oct 22, 2025
121ab52
Merge branch 'master' into fix/modeless-dialog/title-can-be-reactnode
AtsushiM May 13, 2026
8a4a6a1
refactor: PropsWithHTMLAttributesから不要なParameters<FC>[0]を削除
AtsushiM May 13, 2026
a45945c
refactor(ModelessDialog): 型定義を簡潔化
AtsushiM May 13, 2026
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
16 changes: 10 additions & 6 deletions packages/smarthr-ui/src/components/Dialog/ModelessDialog/ModelessDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use client'

import {
type ComponentProps,
type ComponentPropsWithoutRef,
type FC,
type KeyboardEvent,
type MouseEvent,
Expand All @@ -22,7 +22,7 @@ import { type VariantProps, tv } from 'tailwind-variants'
import { useHandleEscape } from '../../../hooks/useHandleEscape'
import { useIntl } from '../../../intl'
import { dialogSize } from '../../../themes/tailwind'
import { Base, type BaseElementProps } from '../../Base'
import { Base } from '../../Base'
import { Button } from '../../Button'
import { Heading } from '../../Heading'
import { FaGripIcon, FaXmarkIcon } from '../../Icon'
Expand All @@ -31,6 +31,7 @@ import { DialogOverlap } from '../DialogOverlap'
import { useDialogPortal } from '../useDialogPortal'

import type { DecoratorsType } from '../../../hooks/useDecorators'
import type { PropsWithHTMLAttributes } from '../../../types/ComponentTypes'
import type { DialogSize } from '../types'

type Props = PropsWithChildren<{
Expand Down Expand Up @@ -135,9 +136,12 @@ const classNameGenerator = tv({
},
})

export const ModelessDialog: FC<
Props & BaseElementProps & VariantProps<typeof classNameGenerator>
> = ({
type ComponentProps = PropsWithHTMLAttributes<

@uknmr uknmr Oct 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

React の ComponentProps と見間違うかも。FC<> に直接書いちゃうか ModelessDialogProps とするかかと思いました。

@Qs-F Qs-F Oct 31, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

reactのComponentProps型はrefを受け取れるかどうか明確ではないため事故の原因になりやすく、慣習的に基本的に使わないべきはずです！あとここはrefを受け取ることが明確ではないため、やるならModelessDialogPropsWithoutRefになりますが、ほかのコンポーネントでも同様の書き方のパターンが出てくるのでComponentPropsが1番良いのではと思っての判断でした

@uknmr uknmr Oct 31, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

他コンポーネントは Props を FC に渡してることが一番多い気がします。
ModelessDialog の Props の名前を変えて、ここを Props とする手もありそうです。
React の ComponentProps 型を使わないのはわかるけど、FC<ComponentProps> だけ見るとうーんってなりました。

@AtsushiM AtsushiM May 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

多分認識のズレが起きてそうで

u: ComponentPropsという名前だとReact.ComponentPropsと勘違いするかもだから他の名前にしたほうが良くない？

ってことを言われている気がします。
慣習的に使わないとしても名前がダブるので他の名前にするかFCに直接渡すほう良い、という意見だと思います。
↑であっているとしたら俺も同意見です。

Props & VariantProps<typeof classNameGenerator>,
'div'
Comment thread
AtsushiM marked this conversation as resolved.
Outdated
>

export const ModelessDialog: FC<ComponentProps> = ({
title,
children,
contentBgColor,
Expand Down Expand Up @@ -193,7 +197,7 @@ export const ModelessDialog: FC<
y: 0,
})
const [draggableBounds, setDraggableBounds] =
useState<ComponentProps<typeof Draggable>['bounds']>()
useState<ComponentPropsWithoutRef<typeof Draggable>['bounds']>()

const decoratorDefaultTexts = useMemo(
() => ({
Expand Down
7 changes: 6 additions & 1 deletion packages/smarthr-ui/src/types/ComponentTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import type { ComponentPropsWithRef, ElementType } from 'react'
import type { ComponentPropsWithRef, ComponentPropsWithoutRef, ElementType, FC } from 'react'

export type ElementRef<T extends ElementType> = ComponentPropsWithRef<T>['ref']

export type ElementRefProps<T extends ElementType> = { ref?: ElementRef<T> }

export type PropsWithHTMLAttributes<
Props extends Parameters<FC>[0],
Comment thread
AtsushiM marked this conversation as resolved.
Outdated
E extends ElementType,
Comment thread
AtsushiM marked this conversation as resolved.
Outdated
> = Props & Omit<ComponentPropsWithoutRef<E>, keyof Props>
Loading