-
Notifications
You must be signed in to change notification settings - Fork 582
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
feat(ResponsiveGrid): test & doc & ts #4748
feat(ResponsiveGrid): test & doc & ts #4748
Conversation
f3b89be
to
1932660
Compare
@@ -36,12 +38,12 @@ const getPadding = padding => { | |||
return paddings; | |||
}; | |||
|
|||
const getMargin = (size, { isNegative, half } = { isNegative: false, half: false }) => { | |||
const getMargin = (size: Spacing, { isNegative, half } = { isNegative: false, half: false }) => { | |||
if (!size) { |
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.
这里明显,size 是支持 undefined 和 null 的。
import { filterUndefinedValue, stripObject } from './util'; | ||
import { env } from '../util'; | ||
import { ResponsiveGridProps, Spacing } from './types'; |
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.
这里没有 import type,可以 npm run type:eslint responsive-grid 检查一下,把所有的 error 和好解的 warning 都解掉。
@@ -72,27 +74,27 @@ const getMargin = (size, { isNegative, half } = { isNegative: false, half: false | |||
return margins; | |||
}; | |||
|
|||
const getChildMargin = spacing => { | |||
return getMargin(spacing, { half: true }); | |||
const getChildMargin = (spacing: Spacing) => { |
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.
同理,这些地方直接调用 getMargin 的地方也是支持的。
{...this.props} | ||
direction="row" | ||
wrap | ||
spacing={gap} |
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.
这里可以 npm run check:types responsive-grid 检查一下,存在 ResponsiveGrid 的 Spacing 和 Box 的 Spacing 对不齐的问题。
device?: 'phone' | 'tablet' | 'desktop'; | ||
/** | ||
* 分为几列, 默认是 12 列 | ||
* @en how many columns in total, 12 by default |
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.
这里没有 defaultValue,其他也要参照 defaultProps 再对照一遍,看有没有遗漏的。
* @api ResponsiveGrid | ||
* @order 0 | ||
*/ | ||
export interface ResponsiveGridProps extends HTMLAttributes<HTMLElement>, CommonProps { |
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.
可以 npm run api responsive-grid,重新生成一下文档,看看有没有哪里是缺失的。
dense?: boolean; | ||
|
||
/** | ||
* @skip |
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.
这些为什么都是 skip 的?
components/util/object.ts
Outdated
/** | ||
* 判断是否为 ReactFragment | ||
* @param component - 传入的组件 | ||
*/ | ||
export function isReactFragment(component?: unknown): boolean { | ||
export function isReactFragment( |
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.
这个 isReactFragment 我感觉原来实现的就有一点问题,既可以是 Fragment,又可以是 Fragment 的 Element 这有什么意义呢?而且从全局搜索来看,这个地方其实就是为了判断是不是 Fragment 的 Element,所以完全可以去掉是 Fragment 的判断,也不需要上面函数重载那么多次了,其实一个类型约束就可以表示清楚。
describe('ResponsiveGrid', () => { | ||
it('should render', () => { | ||
cy.mount(<ResponsiveGrid />); | ||
cy.get('.next-responsive-grid'); |
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.
这里应该加上 .should('exist') 表明这里是一个断言。
No description provided.