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: 图片不需要全部加载完才渲染;修正图片加载过快时首屏不渲染的问题 #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Inori-Lover
Copy link

  1. 移除stateRef的设定,减少阅读阻力
  2. 添加console.count监察渲染压力情况
  3. 移除 forceRefresh 的设定,采用触发scroll事件的形式来触发canvas渲染
  4. 目前在图片仍未加载完成时滚动,会维持着已加载的第一张照片,我不清楚这是什么原因(明明已经clearRect了)但貌似因祸得福:滚动到一半时刷新屏幕看看.jpg
  5. dep是什么东西没看懂,所以就没保留了
  6. 水不到别的了

1. 移除stateRef的设定,减少阅读阻力
2. 添加console.count监察渲染压力情况
3. 移除 forceRefresh 的设定,采用触发scroll事件的形式来触发canvas渲染
4. 目前在图片仍未加载完成时滚动,会维持着已加载的第一张照片,我不清楚这是什么原因(明明已经clearRect了)但貌似因祸得福:滚动到一半时刷新屏幕看看.jpg
5. dep是什么东西没看懂,所以就没保留了
6. 水不到别的了
Copy link
Owner

@zidian257 zidian257 left a comment

Choose a reason for hiding this comment

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

感谢猫娘~~~ you are so nice !~

// 如果觉得useCallback会导致重渲判定那么可以配合useRef进行state缓存,参考:
// https://ahooks.js.org/hooks/advanced/use-persist-fn
drawImageSequence(props, canvasRef, prevState);

Copy link
Owner

Choose a reason for hiding this comment

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

是说,使用

Suggested change
const drawImage = useCallback(() => {
oldDrawImage(props, canvasRef, state)
}, [props, canvasRef, state])

这种形式?这样这个组件就和 React 的状态管理设计强绑定了,也会多不少的 diff 次数,我初衷是,这里的逻辑设计跟 React 是事实上无关的,同样的 draw 逻辑,搬到任何一个地方都适用~ 主要这也是从 @adobe/react-spectrum 学到的,他们也是通过这样的 props, ref, stateRef 入参的设计,快速的适配了 vue 和 svetle

Copy link
Author

@Inori-Lover Inori-Lover Sep 1, 2020

Choose a reason for hiding this comment

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

不是,是这种;

不过框架无关的这个设计有点意思,我阅读代码的时候完全没留意到这个点。我好好再看一下

}
state.current.forceRefresh();
Copy link
Owner

Choose a reason for hiding this comment

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

这个 contact Reverse 的做法可以 merge,比我的好多了(

Copy link
Owner

Choose a reason for hiding this comment

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

但是如果是一个 ref 而不是 state,就明显不用 [...prevState.imageSequence] 再来一遍啊~

Copy link
Author

Choose a reason for hiding this comment

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

主要是我觉得用额外的变量来触发刷新有挺怪的

Copy link
Owner

Choose a reason for hiding this comment

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

额外的变量 isAllLoaded 吗?这个主要是做 guard 用的,这样那几个 listener,resize 和 scroll 就知道自己是否可以 draw。


const imgUrlListLen = imgUrlList.length

imgUrlList.forEach((img, idx) => loadImage(img).then((imageEle) => {
Copy link
Owner

Choose a reason for hiding this comment

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

之前使用 Promise.all 和 promise 数组的设计是说,倘若有图片没有返回来,就不会开启动画。
倘若这里不使用 isAllLoaded,在 draw 的时候不设置 guard,而是 clearRect,譬如在 devTools 开启 slow Mode,就会看到加载过程中进行滚动的时候,出现一会儿有图片,一会儿是背景色的这个问题。

Copy link
Author

@Inori-Lover Inori-Lover Sep 1, 2020

Choose a reason for hiding this comment

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

这个我还真没注意到……那考虑直接跳过这次渲染?

Copy link
Owner

Choose a reason for hiding this comment

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

所以还是推荐使用 isAllLoaded嘛。。作为动画而言,可以额外拿一张图片作为封面,是 fallback 用的图片(这个是设计,连 todo 都没加),如果 preload 挂了的话。

Copy link
Author

Choose a reason for hiding this comment

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

allLoad才显示这个事情我还是接受不了ww

@zidian257
Copy link
Owner

dep 是这样的:

  const callback = React.useMemo(() => ({}), [loaded]);

这样 在 loaded 变化时,也就是全部载入后,会触发一个 resizeHandler,从而触发绘制

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.

2 participants