-
Notifications
You must be signed in to change notification settings - Fork 4
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
Include type information in the package. Partial review of comments #142
Conversation
…e it is false by default).
… false by default).
… false by default).
@@ -65,7 +65,6 @@ type BaseProps = { | |||
open?: boolean; | |||
/** | |||
* openを無視してモーダルを開いたままにするかどうか。アニメーションライブラリとの連携で、ActionHalfModal自身が開閉に関与しない場合に使用 |
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.
Boolean props with a default value of false are more about turning an option on than setting a value,
so I've decided to unify the direction of not writing default values.
@@ -52,10 +49,9 @@ type Props = { | |||
* 幅を指定。fullは後方互換のために残している | |||
* デフォルト<Flex>は横幅いっぱいを専有する。しかし例えば、フレックスコンテナの子要素として配置した場合、横幅が自身の子に合わせて小さくなる。これが不都合の場合に100%とする | |||
*/ | |||
width?: 'full' | Width; | |||
width?: 'full' | CSSWitdh; |
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.
The content is the same. I've changed it to use the direct type.
/** | ||
* 無効状態かどうか | ||
* @default false | ||
*/ | ||
disabled?: boolean; | ||
} & Omit<InputHTMLAttributes<HTMLInputElement>, 'children' | 'onChange'> & | ||
} & Omit<InputHTMLAttributes<HTMLInputElement>, 'children'> & |
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.
I had redefined the props to write an explanation of onChange, but I stopped.
| CSSLengthPercentage | ||
| 'min-content' | ||
| 'max-content' | ||
| 'fit-content' | ||
| `fit-content(${CSSLengthPercentage})` | ||
| CSSVariable; | ||
|
||
export type CSSMinWidth = | ||
| 'auto' | ||
export type CSSMaxWidth = |
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.
The initial value of max-width is none, not auto.
https://developer.mozilla.org/ja/docs/Web/CSS/max-width
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.
The type is incorrect, and the default value for the implementation is none
Changes
Check