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

fix: bugs on pageSize change in List #24514

Merged
merged 14 commits into from Jun 6, 2020
Merged

Conversation

fireairforce
Copy link
Member

@fireairforce fireairforce commented May 27, 2020

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Perfermance optimization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

fix #24501

📝 Changelog

Language Changelog
🇺🇸 English None
🇨🇳 Chinese

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@ant-design-bot
Copy link
Contributor

ant-design-bot commented May 27, 2020

@ant-design-bot
Copy link
Contributor

ant-design-bot commented May 27, 2020

@fireairforce
Copy link
Member Author

fireairforce commented May 27, 2020

感觉这个 LIst 组件原本的测试样例挂的有点多啊...

@hengkx
Copy link
Member

hengkx commented May 27, 2020

yarn test -u

@yoyo837
Copy link
Contributor

yoyo837 commented May 28, 2020

好像不能单纯这么改,props的pagination变化的时候呢?

@xrkffgg
Copy link
Member

xrkffgg commented May 28, 2020

PR 里的 changelog 要填写,不要删。

@fireairforce
Copy link
Member Author

PR 里的 changelog 要填写,不要删。

好的

@fireairforce
Copy link
Member Author

fireairforce commented May 28, 2020

好像不能单纯这么改,props的pagination变化的时候呢?

这个地方按照以前那个逻辑,如果在props里面的pagination 按照 issue 那样写死了 current 参数,点页码切换的时候也只会停在写死的那个页码那里。这个地方本身的逻辑就有些问题。

@fireairforce
Copy link
Member Author

好像不能单纯这么改,props的pagination变化的时候呢?

https://codesandbox.io/s/antd-reproduction-template-52sep?file=/index.js:0-760 具体可以参考这个逻辑

@yoyo837
Copy link
Contributor

yoyo837 commented May 28, 2020

ref: #10357

@fireairforce
Copy link
Member Author

fireairforce commented May 28, 2020

好像不能单纯这么改,props的pagination变化的时候呢?

image
@yoyo837 看了您提供的pr,这个地方觉得还是因为这个问题?我觉得修改这里之后 props中pagination并不会对其他地方造成影响,因为下面的 totalcurrent,pageSize都会保持更新,pagination其他参数的更新也会正常更新。= =

@yoyo837
Copy link
Contributor

yoyo837 commented May 28, 2020

我的意思是,看能不能处理下pagination的更新,顺手改掉这个问题。

@fireairforce
Copy link
Member Author

我的意思是,看能不能处理下pagination的更新,顺手改掉这个问题。

好的,这里我晚上回家研究一下~

@yoyo837
Copy link
Contributor

yoyo837 commented May 28, 2020

PR 里的 changelog 要填写,不要删。

好的

changelog 要面向用户写而非面向开发人员,虽然我也经常不由自主这样干 😂

@fireairforce
Copy link
Member Author

PR 里的 changelog 要填写,不要删。

好的

changelog 要面向用户写而非面向开发人员,虽然我也经常不由自主这样干 😂

哈哈哈,好的,谢谢提醒

@fireairforce
Copy link
Member Author

@yoyo837 那看看这样可行吗? QUQ

@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #24514 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24514   +/-   ##
=======================================
  Coverage   99.21%   99.21%           
=======================================
  Files         365      365           
  Lines        7287     7290    +3     
  Branches     2035     2037    +2     
=======================================
+ Hits         7230     7233    +3     
  Misses         57       57           
Impacted Files Coverage Δ
components/list/index.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dda650c...b9904ae. Read the comment docs.

@fireairforce
Copy link
Member Author

我补一下测试~

@fireairforce
Copy link
Member Author

PTAL

@fireairforce
Copy link
Member Author

fireairforce commented May 29, 2020

貌似感觉修错了bug。。。用户反馈的是改变 pageSize 时候 onChange 函数没有被调用,pageSize 改变时的调用 api 是 onShowSizeChange?

@yoyo837
Copy link
Contributor

yoyo837 commented May 29, 2020

你的意思是 用户需要自行通过 onShowSizeChange 的监听 修改传入的分页信息来达到切换size的效果?

@yoyo837
Copy link
Contributor

yoyo837 commented May 29, 2020

那这样思考的话,就不是bug了。当前就是对的

@afc163
Copy link
Member

afc163 commented May 29, 2020

改变 pageSize 也应该触发 onChange,因为 current page 基本上会变。

@yoyo837
Copy link
Contributor

yoyo837 commented May 29, 2020

改变 pageSize 也应该触发 onChange,因为 current page 基本上会变。

最终的size 任然由用户通过onChange修改传入值吗?

@fireairforce 那就往 onShowSizeChange 没 触发 onChange 的方向改。

@fireairforce
Copy link
Member Author

好的,不好意思,我刚开始可能思考方向想错了,谢谢~

@afc163
Copy link
Member

afc163 commented May 29, 2020

可以参考 Table 的处理。

@yoyo837
Copy link
Contributor

yoyo837 commented May 29, 2020

但是<Pagination /> 组件本身使用时onShowSizeChange只是个独立的回调,不需要根据值重新处理再传入给组件

@fireairforce
Copy link
Member Author

改变 pageSize 也应该触发 onChange,因为 current page 基本上会变。

最终的size 任然由用户通过onChange修改传入值吗?

@fireairforce 那就往 onShowSizeChange 没 触发 onChange 的方向改。

这里的pageSize应该是
image
通过这里切换?

@fireairforce
Copy link
Member Author

done~

@yoyo837
Copy link
Contributor

yoyo837 commented Jun 3, 2020

补一下用例呗,onSizeChange搭配onChange的

@fireairforce
Copy link
Member Author

好,下班晚上回去补~

@fireairforce
Copy link
Member Author

每次用git rebase upstream/xxx 与主库同步之后,推上来就得-f,捂脸.jpg

@fireairforce
Copy link
Member Author

done了~

@fireairforce
Copy link
Member Author

PTAL

@fireairforce
Copy link
Member Author

rebase了


wrapper.find('.ant-select-selector').simulate('mousedown');
wrapper.find('.ant-select-item-option').at(1).simulate('click');
expect(handlePaginationChange).toBeCalled();
Copy link
Member

@afc163 afc163 Jun 5, 2020

Choose a reason for hiding this comment

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

toHaveBeenCalledWidth,把参数也写出来。

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@@ -106,6 +106,11 @@ function List<T>({
return (page: number, pageSize: number) => {
setPaginationCurrent(page);
setPaginationSize(pageSize);
if (eventName === 'onShowSizeChange') {
if (pagination) {
Copy link
Member

Choose a reason for hiding this comment

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

这一行不需要。

Copy link
Contributor

Choose a reason for hiding this comment

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

类型还有false,不能少

Copy link
Member

Choose a reason for hiding this comment

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

@fireairforce
Copy link
Member Author

Done~

@afc163 afc163 merged commit e508ee7 into ant-design:master Jun 6, 2020
@afc163 afc163 mentioned this pull request Jun 6, 2020
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.

pagination onChange not called when the page size changed
6 participants