fix(ModelessDialog): title propsがReactNodeを受け取れるように修正 #5907
Conversation
commit: |
| > = ({ | ||
| type ComponentProps = PropsWithHTMLAttributes< | ||
| Props & VariantProps<typeof classNameGenerator>, | ||
| 'div' |
There was a problem hiding this comment.
もともとはBaseElementProps (HTMLDivElementのAttributesからBaseのPropsをOmitしたもの) を設定していたが、
- 本来はidとariaがほしいくらいですべてのHTMLの属性が欲しいわけではないはず
- 欲しいものだけを絞るべき
- 絞る対応はまた別で行っていくので、一回単純化しておく
- BaseElementPropsをここで使う理由がない
ため、'div' を直接使用しています。
|
|
||
| export type PropsWithHTMLAttributes< | ||
| Props extends Parameters<FC>[0], | ||
| E extends ElementType, |
There was a problem hiding this comment.
いい感じの被らない型パラメーター名が思いつかなかったので…
| export type ElementRefProps<T extends ElementType> = { ref?: ElementRef<T> } | ||
|
|
||
| export type PropsWithHTMLAttributes< | ||
| Props extends Parameters<FC>[0], |
There was a problem hiding this comment.
Parameters<FC>[0] が効いてなさそうに見えました。
| Props extends Parameters<FC>[0], | |
| Props, |
| export const ModelessDialog: FC< | ||
| Props & BaseElementProps & VariantProps<typeof classNameGenerator> | ||
| > = ({ | ||
| type ComponentProps = PropsWithHTMLAttributes< |
There was a problem hiding this comment.
React の ComponentProps と見間違うかも。FC<> に直接書いちゃうか ModelessDialogProps とするかかと思いました。
There was a problem hiding this comment.
reactのComponentProps型はrefを受け取れるかどうか明確ではないため事故の原因になりやすく、慣習的に基本的に使わないべきはずです！あとここはrefを受け取ることが明確ではないため、やるならModelessDialogPropsWithoutRefになりますが、ほかのコンポーネントでも同様の書き方のパターンが出てくるのでComponentPropsが1番良いのではと思っての判断でした
There was a problem hiding this comment.
他コンポーネントは Props を FC に渡してることが一番多い気がします。
ModelessDialog の Props の名前を変えて、ここを Props とする手もありそうです。
React の ComponentProps 型を使わないのはわかるけど、FC<ComponentProps> だけ見るとうーんってなりました。
関連URL
https://smarthr.atlassian.net/browse/SHRUI-1330
概要
ModelessDialogのtitleが意図せずstringのみを許容する様になっていたのを修正
変更内容
確認方法