Skip to content

fix: add requestAnimationFrame to resizeObserver(#23956); #319

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 15, 2020
Merged

fix: add requestAnimationFrame to resizeObserver(#23956); #319

merged 1 commit into from
May 15, 2020

Conversation

@kerm1it
Copy link
Member

@kerm1it kerm1it commented May 14, 2020

#23956
目前加的防抖时间是100毫秒。

@codecov
Copy link

codecov bot commented May 14, 2020 •
edited
Loading

Codecov Report

Merging #319 into master will decrease coverage by 0.45%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
- Coverage   93.84%   93.39%   -0.46%     
==========================================
  Files          11       11              
  Lines         813      818       +5     
  Branches      236      236              
==========================================
+ Hits          763      764       +1     
- Misses         50       54       +4
Impacted Files Coverage Δ
src/DOMWrap.tsx 80.30% <16.66%> (-2.38%) ⬇️

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 099bcec...cafa496. Read the comment docs.

@kerm1it kerm1it mentioned this pull request May 14, 2020
Closed
1 task
afc163
afc163 reviewed May 14, 2020
View reviewed changes
src/DOMWrap.tsx Outdated

componentDidMount() {
this.setChildrenWidthAndResize();
this.setChildrenWidthAndResize = debounce(
Copy link
Member

@afc163 afc163 May 14, 2020

Choose a reason for hiding this comment

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

组件卸载的时候要清除掉副作用。

afc163
afc163 reviewed May 14, 2020
View reviewed changes
src/DOMWrap.tsx Outdated
componentDidMount() {
this.setChildrenWidthAndResize();
this.setChildrenWidthAndResize = throttle(
this.setChildrenWidthAndResize,
Copy link
Member

@afc163 afc163 May 14, 2020

Choose a reason for hiding this comment

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

这个写法有点奇怪，不要覆盖 this.setChildrenWidthAndResize。

Copy link
Member

@afc163 afc163 May 14, 2020

Choose a reason for hiding this comment

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

想了一下不要用 throttle，直接上 requestAnimationFrame 就行了。

afc163
afc163 reviewed May 14, 2020
View reviewed changes
@kerm1it
Copy link
Member Author

kerm1it commented May 14, 2020

测试失败了，我看看。

afc163
afc163 reviewed May 14, 2020
View reviewed changes
src/DOMWrap.tsx
cancelAnimationFrame(cancelFrameId);
this.cancelFrameId = requestAnimationFrame(
this.setChildrenWidthAndResize,
);
Copy link
Member

@afc163 afc163 May 14, 2020

Choose a reason for hiding this comment

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

记得在 componentWillUnmount 里 cancelAnimationFrame 掉。

Copy link
Member

@afc163 afc163 May 14, 2020

Choose a reason for hiding this comment

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

这样写性能提升效果如何？

Copy link
Member Author

@kerm1it kerm1it May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK

Copy link
Member Author

@kerm1it kerm1it May 14, 2020

Choose a reason for hiding this comment

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

这样写了后效果可以，和节流的效果差不多，就是从整个菜单隐藏到显示时，最后的...按钮还是会闪一下(延迟出现)。

@kerm1it
Copy link
Member Author

kerm1it commented May 14, 2020

用了requestAnimationFrame后是不是初始化时还是得调用一下？不然测试报错

@kerm1it
Copy link
Member Author

kerm1it commented May 14, 2020

735ex-0ojhs
这是现在的效果

@afc163
Copy link
Member

afc163 commented May 14, 2020

用了requestAnimationFrame后是不是初始化时还是得调用一下？不然测试报错

可以试试，不过我担心还是会影响性能。

@kerm1it
Copy link
Member Author

kerm1it commented May 14, 2020

用了requestAnimationFrame后是不是初始化时还是得调用一下？不然测试报错

可以试试，不过我担心还是会影响性能。

我试了，得加上，不然初始化的时候就会闪一下。

@kerm1it
Copy link
Member Author

kerm1it commented May 14, 2020 •
edited
Loading

这个怎么覆盖测试，想了半天不知道怎么写？

@afc163
Copy link
Member

afc163 commented May 15, 2020

可能要 await sleep(100); 之类的。

@kerm1it
Copy link
Member Author

kerm1it commented May 15, 2020 

const { cancelFrameId } = this;
cancelAnimationFrame(cancelFrameId);
this.cancelFrameId = requestAnimationFrame(
   this.setChildrenWidthAndResize,
);

这段怎么覆盖呢？我想着是检测setChildrenWidthAndResize的调用次数，但是好像不太行呢。

@kerm1it kerm1it changed the title fix: add debounce to resizeObserver(#23956); fix: addrequestAnimationFrame to resizeObserver(#23956); May 15, 2020
@kerm1it kerm1it changed the title fix: addrequestAnimationFrame to resizeObserver(#23956); fix: add requestAnimationFrame to resizeObserver(#23956); May 15, 2020
@kerm1it
Copy link
Member Author

kerm1it commented May 15, 2020

@afc163 这个咱们怎么弄呢？现在我的改动和效果就是我发的那样，就是新加的那几行测试覆盖不到。

@afc163
Copy link
Member

afc163 commented May 15, 2020

不好测先跳过好了

@afc163 afc163 merged commit 39fc3ed into react-component:master May 15, 2020
@afc163
Copy link
Member

afc163 commented May 15, 2020

谨慎起见，我发了 8.2.0，到 antd 里升级一下测试。

@kerm1it
Copy link
Member Author

kerm1it commented May 15, 2020

不好测先跳过好了

OK, 更新后建议你也先在本地看看效果有什么问题

@afc163
Copy link
Member

afc163 commented May 15, 2020

npm 貌似没法发布。。

@kerm1it
Copy link
Member Author

kerm1it commented May 15, 2020

npm 貌似没法发布。。

有什么问题？

@afc163
Copy link
Member

afc163 commented May 15, 2020

npm 挂了。

@kerm1it
Copy link
Member Author

kerm1it commented May 15, 2020

npm 挂了。

image
好像是的

@afc163
Copy link
Member

afc163 commented May 15, 2020

可以了，已发布。

@kerm1it
Copy link
Member Author

kerm1it commented May 15, 2020

13u7d-8upa8
我试了，效果可以，就是从隐藏到显示切换时，会闪一下。

@afc163
Copy link
Member

afc163 commented May 17, 2020

给 antd 发 PR 吧

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afc163 afc163 afc163 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kerm1it @afc163