-
Notifications
You must be signed in to change notification settings - Fork 141
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
chore: decoratorsに対する処理を共通化できるよう、useDecoratorsを定義する #5326
base: master
Are you sure you want to change the base?
Conversation
93b6e65
to
2f643da
Compare
2f643da
to
0d284e1
Compare
commit: |
@@ -1,7 +1,7 @@ | |||
import React, { FC, KeyboardEventHandler, useCallback, useMemo } from 'react' | |||
import { tv } from 'tailwind-variants' | |||
|
|||
import { DecoratorsType } from '../../types' | |||
import { type DecoratorsType } from '../../libs/decorator' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type の場合は type を強制する lint ってないのかしら。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありそう。
全プロダクトonにしてもいいのでは? (可読性かなり上がりそう)
https://typescript-eslint.io/rules/consistent-type-imports/
@@ -21,6 +21,30 @@ export default { | |||
<Textarea {...args} maxLetters={5} value="テキスト" /> | |||
<Textarea {...args} maxLetters={5} value="テキストエ" /> | |||
<Textarea {...args} maxLetters={5} value="テキストエリア" /> | |||
<Textarea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo: decorator.ts の動作が担保されているのであれば、各コンポーネントで VRT に足す必要はなさそうに思いました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decorator.ts の動作が担保されているのであれば、各コンポーネントで VRT に足す必要はなさそうに思いました。
decorator自体の動作はそうですが、decoratorがどこに反映されるのか?は確認できないのでVRTに含めておくほうが良くないです?
特に今回のような処理が外部で行われる様になる場合はバグったことが検知しにくくなるのでテストしておきたい所存
export type DecoratorType = (text: string) => ReactNode | ||
|
||
export const useDecorators = <T extends string>( | ||
defaultTexts: { [K in T]: string }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo: デフォルトテキスト、というかは変換表だなと思った。translateTable
的な。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変換表
いや、指定無い場合の文字列のまとまりなんでtranslateTableだと役割の一部しか表せてない気がします
役割も翻訳だけじゃなくて違う文字列にすることも有り得るのでtranslateは不適切っぽい。
…e-add-use-decorators
7c70320
to
606ccb4
Compare
606ccb4
to
7f05277
Compare
…e-add-use-decorators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useDecoratorsとTextarea以外は見てないですが良さそうな気はしました!
return defaultTexts | ||
} | ||
|
||
const decorated = {} as { [K in T]: ReactNode } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asちょっと怖い気がするので
const decorated = {} as { [K in T]: ReactNode } | |
const decorated: { [K in T]: ReactNode } = {} |
とか、satisfiesとかで代用できないんでしたっけ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変数に型として設定してしまうと、 {}
は明らかに型違反なので駄目っすね...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
例えば
const decorated: { [key: string]: ReactNode } = {}
....
return decorated as { [K in T]: ReactNode }
とすればもうちょいわかりやすいかもですがあまり意味はなさそう...
関連URL
概要
変更内容
確認方法