Canvas: Show base color / blurhash while resource is loading #11849
Conversation
|
Seems to work well already! |
|
Added a fix for #10167, as it was really effecting my testing in safari. |
Screen.Recording.2022-07-01.at.16.07.09.movAs noted above, here is the issue in safari. It is none issue, but it make testing this PR a little harder. |
|
Size Change: +223 B (0%) Total Size: 2.65 MB
ℹ️ View Unchanged
|
|
Plugin builds for 7fd25aa are ready 🛎️!
|
There was a problem hiding this comment.
Looks good, just one thing to fix:
width and height passed to BlurhashContainer is taken from the original image (big one) so the canvas blurhash looks different from the thumbnail blurhash.
I think we can remove the width and height props from the BlurhashContainer as they are optional and the parent will take care of the rest, or use width=100%, height=auto.
Example (1st load - current, 2nd - fixed):
blur.mp4
@merapi The height and width are required here. Depending on these, the blurhash looks different. But you are correct that the height and width of the resource are the wrong. I have tweaked the logic to use the size of the element, which improves performance and looks a lot better. |
Looks like the docs are not updated? it defaults to It is a bit misleading but width/height is only used in div style and what actually impacts performance is the resolution props: <div
{...rest}
style={{ display: 'inline-block', height, width, ...style, position: 'relative' }}
>
<BlurhashCanvas
hash={hash}
height={resolutionY}
width={resolutionX}
punch={punch}
style={canvasStyle}
/>
</div>Either solution will make it look correct 👍🏻 |
|
The height and width are used to generate the canvas. See https://github.com/woltapp/react-blurhash/blob/master/src/BlurhashCanvas.tsx |
Nope, we are not using |
But blurhash does blurhashcanvas - https://github.com/woltapp/react-blurhash/blob/458b44880845085066e5640bdc5a24a51f077002/src/Blurhash.tsx#L53-L59 |
Correct, but look how it passes the props.
|
Right, I have passed x and y props. |
That is unnecessary (actually degrading the performance).
Right now my test image is rendered with resolution=336, it takes ~62ms to render, while with recommended/default resolution=32 it takes ~1ms. I have a little tool for performance testing and there is no point to go above >32 in resolution because there is no visual difference, it just takes longer to render (we encode the image with components=4 so there is not much data to work with).
So the best settings are: resolution=32 (default), width/height can be undefined/auto or actual box dimensions - no difference. |
Height and width have been removed. |
Context
Summary
Show base color / blurhash based placeholder for element on canvas.
Relevant Technical Choices
To-do
User-facing changes
Screen.Recording.2022-06-30.at.16.57.02.mov
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #10258