Skip to content
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

timeline 组件 #47

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Conversation

MuYunyun
Copy link
Member

No description provided.

@MuYunyun MuYunyun changed the title [WIP] timeline timeline 时间轴组件 May 21, 2019
@MuYunyun MuYunyun changed the title timeline 时间轴组件 timeline 组件 May 21, 2019
components/Timeline/__test__/index.test.tsx Show resolved Hide resolved
components/Timeline/timeline.tsx Outdated Show resolved Hide resolved
components/Timeline/timeline.tsx Outdated Show resolved Hide resolved
types/timeline.d.ts Show resolved Hide resolved
Copy link
Contributor

@anyexinglu anyexinglu left a comment

Choose a reason for hiding this comment

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

接口定义和api文档让人晕眩

components/Timeline/timeline.tsx Outdated Show resolved Hide resolved
components/Timeline/timeline.tsx Outdated Show resolved Hide resolved
className: cx(`${prefixCls}-item`, {
[`${prefixCls}-item-last`]: count - 1 === index
}),
ifCurrent: current === index,
Copy link
Contributor

Choose a reason for hiding this comment

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

isCurrent?

Copy link
Member Author

Choose a reason for hiding this comment

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

isCurrent 作为 Timeline 组件到 Timeline.Item 组件的一个传参,表示当前时间轴哪个节点高亮。

Copy link
Contributor

Choose a reason for hiding this comment

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

我的意思是:你叫做ifCurrent,是不是isCurrent比较规范?

function renderTimeline() {
if (options && options.length) {
return options.map((child, index) => {
const { content, lineHeight, dot } = child
Copy link
Contributor

Choose a reason for hiding this comment

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

lineHeight为啥单独暴露出来,
如果要支持不同尺寸,应该是大中小的size(fontSize 搭配 lineHeight)

Copy link
Member Author

Choose a reason for hiding this comment

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

lineHeight 表示线条的高度, 可能命名和 line-height 重名了。不知道有没有其它名字推荐。比如当 lineHeight 为 0 时,相应的时间轴就’消失‘了。

image

Copy link
Contributor

Choose a reason for hiding this comment

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

lineHeight感觉不如lineStyle好,可以自定义宽度 高度 颜色啥的

Copy link
Contributor

Choose a reason for hiding this comment

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

主要是歧义比较严重

return options.map((child, index) => {
const { content, lineHeight, dot } = child
return cloneElement(
<TimelineItem dot={dot} lineHeight={lineHeight} key={index}>
Copy link
Contributor

Choose a reason for hiding this comment

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

key=index不推荐?

Copy link
Member Author

Choose a reason for hiding this comment

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

因为这里没有数组首位增删的操作, 所以不会造成 index 作为 key 造成的 bug, 不过还可以用什么作为 key 呢, 除了随机数。

| dot | 自定义时间轴点 | React.ReactNode | -- |
| lineHeight | 线条高度, 单位百分比 | Number | 100 |

## TimeLineConfigProps Api
Copy link
Contributor

Choose a reason for hiding this comment

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

文档和interface里不一致,不便于理解呐

Copy link
Member Author

Choose a reason for hiding this comment

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

已删减

| ------------ | :----------------------------: | :-------------------: | --------: |
| current | 当前高亮的节点 | number | 0 |
| currentColor | 高亮节点的颜色 | string | '#1199EE' |
| options | 支持配置模式或者 children 模式 | TimeLineConfigProps[] | -- |
Copy link
Contributor

Choose a reason for hiding this comment

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

支持children模式的话,Timeline Api里应该有个children?(另外,我们best pc不需要children哈)

Copy link
Member Author

Choose a reason for hiding this comment

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

已添加

| ---------- | :------------------: | :-------------: | -----: |
| dot | 自定义时间轴点 | React.ReactNode | -- |
| lineHeight | 线条高度, 单位百分比 | Number | 100 |

Copy link
Contributor

Choose a reason for hiding this comment

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

content呢

Copy link
Member Author

Choose a reason for hiding this comment

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

已添加

/* 可以使用 options 或者 Item 两种方式 */
options?: TimelineItemConfig[]
/* 子节点 */
children?: React.ReactNode
Copy link
Contributor

Choose a reason for hiding this comment

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

@MuYunyun MuYunyun requested a review from anyexinglu May 23, 2019 16:45
className: cx(`${prefixCls}-item`, {
[`${prefixCls}-item-last`]: count - 1 === index
}),
ifCurrent: current === index,
Copy link
Contributor

Choose a reason for hiding this comment

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

我的意思是:你叫做ifCurrent,是不是isCurrent比较规范?

</TimelineItem>
</Timeline>
)
expect(Timeline4).toMatchSnapshot()
Copy link
Contributor

Choose a reason for hiding this comment

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

都是snapshot,感觉等于没测👀

function renderTimeline() {
if (options && options.length) {
return options.map((child, index) => {
const { content, lineHeight, dot } = child
Copy link
Contributor

Choose a reason for hiding this comment

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

lineHeight感觉不如lineStyle好,可以自定义宽度 高度 颜色啥的

function renderTimeline() {
if (options && options.length) {
return options.map((child, index) => {
const { content, lineHeight, dot } = child
Copy link
Contributor

Choose a reason for hiding this comment

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

主要是歧义比较严重

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants