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

If <Table /> is hidden, don't recalculate rowHeight. #122

Merged
merged 4 commits into from
Feb 17, 2017

Conversation

ystarlongzi
Copy link
Contributor

fix ant-design #4836

然后在写测试时发现使用enzyme 渲染组件,无法获取元素的宽高,所以就没补上测试。

src/Table.jsx Outdated
const tableRect = this.tableNode.getBoundingClientRect();
// If tableNode's height less than 0, suppose it is hidden and don't recalculate rowHeight.
// see: https://github.com/ant-design/ant-design/issues/4836
if (tableRect.height <= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

有没可能 tableRect 或 tableRect.height 是 undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

有可能,IE8或更低浏览器里返回的 rect 没有 width 和 height 属性。如果要兼容 IE8- 的话,将获取高度的方式改为rect.bottom - rect.top应该可行。

Copy link
Member

Choose a reason for hiding this comment

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

tableRect.height && tableRect <= 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

条件判断是不是应该这样?tableRect.height !== undefined && tableRect.height <= 0

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 83.226% when pulling 9cda412 on ystarlongzi:master into d297856 on react-component:master.

@ystarlongzi
Copy link
Contributor Author

@afc163 求教

  1. rc-tools run lint 失败,要如何解决呢?
  2. 这里的测试要如何写呢?

@yesmeck
Copy link
Member

yesmeck commented Feb 15, 2017

CI 我调整了, rebase 下 master 吧。

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 83.226% when pulling 6a98774 on ystarlongzi:master into 78fbd1a on react-component:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 83.226% when pulling cec05f6 on ystarlongzi:master into 78fbd1a on react-component:master.

@ystarlongzi
Copy link
Contributor Author

ystarlongzi commented Feb 15, 2017

@afc163 @yesmeck
还有测试覆盖率下降了 2.5% 😂,目前我也没找到方案解决

@benjycui
Copy link
Member

@ystarlongzi jsdom 获取 DOM 元素宽高的问题,可以这样 mock https://github.com/react-component/slider/blob/master/tests/common/createSlider.test.js#L90

src/Table.jsx Outdated
@@ -587,6 +587,12 @@ const Table = React.createClass({
},

syncFixedTableRowHeight() {
const tableRect = this.tableNode.getBoundingClientRect();
Copy link
Member

Choose a reason for hiding this comment

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

单测里面 tableNode.getBoundingClientRect === mock function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,我试下,谢谢

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 85.281% when pulling bd018f7 on ystarlongzi:master into 78fbd1a on react-component:master.

@ystarlongzi
Copy link
Contributor Author

这个测试失败怎么解决呢?Table.jsx#L639 处的 this 变成 undefined 了?

但是本地运行 npm run testnpm run coverage 也都正常。

本地环境如下:

$ node -v
v6.5.0
$ npm -v
v3.10.3

@yesmeck
Copy link
Member

yesmeck commented Feb 16, 2017

再 rebase 下吧。

@coveralls
Copy link

Coverage Status

Coverage increased (+5.5%) to 90.693% when pulling 843f672 on ystarlongzi:master into a90c88c on react-component:master.

@ystarlongzi
Copy link
Contributor Author

@yesmeck 不是很明白,handleBodyScroll 函数里的 this 指向有在哪个环节被改掉了?

@yesmeck
Copy link
Member

yesmeck commented Feb 16, 2017

我把 React.createClass 改成 ES6 class 了,自动绑定没有了。

@ystarlongzi
Copy link
Contributor Author

ystarlongzi commented Feb 16, 2017 via email

@yesmeck yesmeck merged commit d912793 into react-component:master Feb 17, 2017
@afc163
Copy link
Member

afc163 commented Feb 17, 2017

发个 patch ?

@yesmeck
Copy link
Member

yesmeck commented Feb 17, 2017

@ @ 等我周末再补点测试

@afc163
Copy link
Member

afc163 commented Feb 17, 2017

这么晚还在..

@yesmeck
Copy link
Member

yesmeck commented Feb 18, 2017

Released 5.2.10

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.

<Table /> 在某些场景下,固定列行高度变为 auto,导致行错位
5 participants