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
wants to merge 2 commits into
base: master
Choose a base branch
from
+16 −7
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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<
Copy link
Contributor

@uknmr uknmr Oct 31, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Qs-F Qs-F Oct 31, 2025

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番良いのではと思っての判断でした

Copy link
Contributor

@uknmr uknmr Oct 31, 2025

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> だけ見るとうーんってなりました。

Props & VariantProps<typeof classNameGenerator>,
'div'
Copy link
Contributor Author

@Qs-F Qs-F Oct 22, 2025

Choose a reason for hiding this comment

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

もともとはBaseElementProps (HTMLDivElementのAttributesからBaseのPropsをOmitしたもの) を設定していたが、

  • 本来はidとariaがほしいくらいですべてのHTMLの属性が欲しいわけではないはず
    • 欲しいものだけを絞るべき
    • 絞る対応はまた別で行っていくので、一回単純化しておく
  • BaseElementPropsをここで使う理由がない

ため、'div' を直接使用しています。

>

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],
Copy link
Contributor

@uknmr uknmr Oct 31, 2025

Choose a reason for hiding this comment

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

Parameters<FC>[0] が効いてなさそうに見えました。

Suggested change
Props extends Parameters<FC>[0],
Props,

E extends ElementType,
Copy link
Contributor Author

@Qs-F Qs-F Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

いい感じの被らない型パラメーター名が思いつかなかったので…

> = Props & Omit<ComponentPropsWithoutRef<E>, keyof Props>
Loading