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

feat: renderFooterInTable toggle to render footer as <tfoot /> #191

Closed
wants to merge 0 commits into from
Closed

Conversation

hackape
Copy link
Contributor

@hackape hackape commented Feb 26, 2018

No description provided.

return <Cell key={key || dataIndex}>{text}</Cell>
})

return <Row>{cells}</Row>
Copy link
Member

@yesmeck yesmeck Feb 26, 2018

Choose a reason for hiding this comment

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

Is returning <Row /> required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. Only when you set renderFooterInTable to true should you return <Row />.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep the original footer API untouched. Pass extra args to it, but only takes effect when you toggle renderFooterInTable on.

@hackape
Copy link
Contributor Author

hackape commented Feb 26, 2018

I only intent to provide a way to put the footer right inside the table's <tfoot />. User can still return whatever they want from footer={(...args) => Element}. If renderFooterInTable === true, that <Element /> will be insert into <tfoot /> .

As of args, it contains (currentData, columns, Row, Cell). columns includes an extra { key: 'expand-icon-placeholder' } when needed, Row is components.footer.row, Cell the same. It's still up to user to construct the right return value. One can still put an <div /> into <tfoot />, react will complain, but browser will let it through.

@hackape
Copy link
Contributor Author

hackape commented Feb 27, 2018

@yesmeck Any feedback?

src/FootTable.js Outdated
style={footStyle}
onScroll={handleBodyScrollLeft}
>
<BaseTable
Copy link
Member

Choose a reason for hiding this comment

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

Why render tfoot to a separator BaseTable, can we just render it in the BaseTable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason is to keep its behavior consistent to Header's. When you turn { scroll: y } on, Header is fixed, most likely user want the Footer to be fixed too.

@yesmeck
Copy link
Member

yesmeck commented Feb 27, 2018

screen shot 2018-02-27 at 14 47 36

It seems to render tfoot to a sparator BaseTable break the hover state syncing.

@hackape
Copy link
Contributor Author

hackape commented Feb 27, 2018

Hmm, I didn't notice there's hover state syncing in there...🤔 Lemme check.

@yesmeck
Copy link
Member

yesmeck commented Feb 27, 2018

Could you consider the proposal I wrote in ant-design/ant-design#5710 (comment)

@hackape
Copy link
Contributor Author

hackape commented Feb 27, 2018

I see. Actually, I don't think there should be any hover state on footer at all. Current behavior is simply an side effect of css tr:hover style. Should just limit css to take effect on tbody > tr only, leave hover state on footer to user.

@hackape
Copy link
Contributor Author

hackape commented Feb 27, 2018

换中文吧?我同意你的这个 API 加入方式更好些。我会改成用这个来实现。

除了这点,现在的这个 fixed footer 的实现你有什么要 comment 的吗?

@hackape
Copy link
Contributor Author

hackape commented Feb 27, 2018

另外我建议命名为 column = { tfoot: (data) => } 更好吧,进一步避免跟 footer 的混淆。

@yesmeck
Copy link
Member

yesmeck commented Feb 27, 2018

就叫 “footer” 好了,我们也没有把 “row” 叫 “tr”。

@hackape
Copy link
Contributor Author

hackape commented Feb 27, 2018

ok,命名你们定。

@yesmeck
Copy link
Member

yesmeck commented Feb 27, 2018

其他看起来 ok

@yesmeck
Copy link
Member

yesmeck commented Feb 28, 2018

又考虑了下,footer 其实对应的就是 title。所以 footer 应该也支持直接写 ReactNode

const columns = [{
  title: 'Price',
  footer: `Amount: ${amount}`,
}]

@Nokecy
Copy link
Contributor

Nokecy commented Mar 22, 2018

@hackape 这个功能咋样呀

@hackape
Copy link
Contributor Author

hackape commented Mar 22, 2018

这段时间忙就给放下了,我今明两天应该能提个新的上来。

@xgj1988
Copy link

xgj1988 commented Jul 10, 2018

@hackape column footer 这个是否 可以在antd table 里面使用?

@hackape
Copy link
Contributor Author

hackape commented Jul 11, 2018

@xgj1988 这个 PR 被 revert 掉了,所以现在没有这功能。官方建议用自定义的 table.props.components.body.wrapper 来解决这问题。

@xgj1988
Copy link

xgj1988 commented Jul 11, 2018

@hackape 用哪个也有问题。 https://codesandbox.io/s/xjpkx9rzzw 这是别人的demo 。这里有两个问题。
1:合计列不能固定在底部。2:加上fixed 之后,样式全完乱了。

@hackape
Copy link
Contributor Author

hackape commented Jul 11, 2018

@xgj1988 不是让你直接用这个 demo 的实现,这只是给你提供一个可行思路。现在的情况就是官方团队不想增加 table 的复杂度,这种高级用法需要你自己去实现,而相应的接口就是 table.props.components.body.wrapper.

@pilot-study-test
Copy link

打印需要用到tfoot,官方却没有这种打印需求。

@leejaen
Copy link

leejaen commented Oct 11, 2018

tfoot功能应该属于硬需求,不知为何就是不给提供,即使别人已经实现了PR。。。再即使,当前的footer有明显的样式问题也要保留,很明显的footer的边框线上下不能对齐 @afc163 @yesmeck @hackape #195

@hackape
Copy link
Contributor Author

hackape commented Oct 12, 2018

@leejaen 你也可以尝试提一个完善的 PR

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.

None yet

6 participants