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: layout shift when uploading images #6930

Merged
merged 11 commits into from
Jun 14, 2024

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Jun 13, 2024

Description

This branch started out as an attempt at fixing a React 19 issue where the Image Tool buttons failed to render, and the assumption was that the root cause were that BaseImageInput component were still a class component and had bugs in it implementation that linters and typechecks were unable to pick up.
After finishing the refactor to a Functional Component the issue were still there, and turned out to be a problem in @sanity/ui that's now fixed.
Since the refactor to a functional component is still valuable (easier to optimize, allows using hooks instead of hacks like <ImperativeToast/>, etc) I kept going and uncovered a bug while testing that the refactor doesn't have regressions:

before.image.upload.fix.trimmed.mov

This layout shift issue is recorded on https://test-studio.sanity.build, it's easier to reproduce if slow network conditions are simulated:

image

This PR includes a fix, here's the new behaviour:

after.image.upload.fix.trimmed.mov

What to review

Does the new code structure make sense? The goal was to reuse existing names as much as possible, and to co-locate things.

Testing

Unsure if existing test suites are enough.
Personally I've tested all the flows I could think of and they're passing:

  • uploading an image on an empty input
  • updating hotspot and crop
  • clearing an image and then upload another image
  • drag to upload image
  • copy image into input
  • paste image on existing image with different dimensions to test layout shift
  • selecting new images from the asset source browser
  • tested error states (uploading .avif images, interrupt uploads)
  • custom asset sources

Notes for release

Reduce layout shift when uploading images that replace existing image assets.

Copy link

vercel bot commented Jun 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 9:00am
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 9:00am
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 9:00am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 9:00am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 9:00am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Jun 14, 2024 9:00am

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Jun 13, 2024

Component Testing Report Updated Jun 14, 2024 9:04 AM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 37s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 25s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 31s 11 7 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 52s 1 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 35s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 14s 20 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 2s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 6s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 20s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 28s 12 0 0

@stipsan stipsan changed the title refactor: change ImageInput from PureComponent to a memo(React.FC) fix: layout shift when uploading images Jun 13, 2024
@stipsan stipsan force-pushed the fix-missing-image-input-buttons-in-react-19 branch from 3471702 to 52ba912 Compare June 13, 2024 14:55
@stipsan stipsan marked this pull request as ready for review June 13, 2024 14:56
@stipsan stipsan requested review from a team as code owners June 13, 2024 14:56
@stipsan stipsan requested review from pedrobonamin and bjoerge and removed request for a team June 13, 2024 14:56
@stipsan stipsan enabled auto-merge June 13, 2024 14:57
Copy link
Contributor

@juice49 juice49 left a comment

Choose a reason for hiding this comment

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

Thank you! Code changes and Studio changes when testing look good to me.

@stipsan stipsan added this pull request to the merge queue Jun 14, 2024
Merged via the queue into next with commit ae142d4 Jun 14, 2024
42 of 43 checks passed
@stipsan stipsan deleted the fix-missing-image-input-buttons-in-react-19 branch June 14, 2024 10:45
cngonzalez pushed a commit that referenced this pull request Jun 18, 2024
* refactor: extract `ImageInput->renderPreview` into FC

* refactor: extract `ImageInput-> renderHotspotInput ` into FC

* refactor: extract `ImageInput-> renderAsset ` into FC

* refactor: extract `ImageInput-> renderUploadPlaceholder ` into FC

* refactor: extract `ImageInput->renderAssetSource` into FC

* refactor: extract `ImageInput->renderAssetMenu` into FC

* refactor: use hooks instead of HOC

* refactor: extract `ImageInput->renderBrowser` into FC

* refactor: replace `BaseImageInput` class component with FC

* chore: fix depcheck error

* fix: remove layout jumps while uploading images
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.

None yet

2 participants