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(sanity): prevent layout shifts in image input #7535

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

juice49
Copy link
Contributor

@juice49 juice49 commented Sep 22, 2024

Description

This branch introduces refinements to the image input preview, primarily to address the layout shift reported in #7337.

There are two scenarios in which layout shift can occur:

  1. When the image is loading.
  2. When the selected image state transitions to the uploading state, and the uploading state subsequently transitions to the new image. There has been a previous attempt to fix this (fix: layout shift when uploading images #6930), but I believe this may be causing the layout shift when the image loads.

This branch removes the ref added in #6930. Instead of attempting to preserve the last known height of the image preview DOM node using a ref, the known image dimensions are set as CSS custom properties on the outer container of the image input. Both the image preview and upload state UI use CSS to match the aspect ratio of the selected image, eliminating both layout shifts noted above.

Importantly, the value of the image field reflects the previously selected image while the next image is uploading. This means the upload state UI can match the dimensions of the previously selected image, addressing layout shift when uploading a new image in a simpler manner than before.

This branch also introduces a couple of other refinements to the image input:

  1. Fixes incorrect use of light colour scheme in overlay components, regardless of whether the user has light mode active. In dark mode, this caused a flash of a light placeholder block, which looks janky.
  2. Allows the image preview's block size to extend to 30vh. I believe there was previously an attempt to achieve this, but the block size was ultimately limited to a rem value in CSS. Moving the calculations to CSS makes it simpler to allow the block size to extend to a vh value, while collapsing to the intrinsic height of the image if it's less than 30vh.

Demo

Before (video) After (video)
https://github.com/user-attachments/assets/f79bb9f8-cf62-4f51-a1d0-8bd2b3e9cdf7 https://github.com/user-attachments/assets/8df6455a-17b4-4d08-a049-521576fca43d
Noticeable layout jitter when loading the image, and a flash of the light colour mode when dark mode is active make images quite janky. After these changes, images are much more stable.

Images now have a maximum height of 30vh.

Before After
before-max-height after-max-height

We still preserve the aspect ratio of the current image to prevent layout shifts when uploading a new one, which was first addressed by Cody in #6930. Now we achieve it in a simpler way.

after-upload.mov

What to review

Does the approach make sense?

Testing

  • Tested various image sizes and aspect ratios.
  • Tested various image states (image present, image uploading, image upload failed).
  • Tested various viewport sizes.
  • Tested in Safari, Chrome, and Firefox.

Notes for release

  • Eliminated layout shifts in image fields.
  • Fixed incorrect usage of light mode in image field overlays when dark mode is active.

Copy link

vercel bot commented Sep 22, 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 Oct 1, 2024 9:26am
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 1, 2024 9:26am
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 1, 2024 9:26am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 1, 2024 9:26am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 1, 2024 9:26am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Oct 1, 2024 9:26am

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Sep 22, 2024

Component Testing Report Updated Oct 1, 2024 9:25 AM (UTC)

✅ All Tests Passed -- expand for details
File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 46s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 31s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 37s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 3m 0s 0 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 44s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 39s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 15s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 9s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 26s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 35s 12 0 0
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 0s 0 3 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 0s 0 3 0

@juice49 juice49 force-pushed the fix/sdx-1579/image-input-layout-shift branch from a2cf780 to 2eb20ff Compare September 30, 2024 13:24
@juice49 juice49 marked this pull request as ready for review September 30, 2024 14:20
@juice49 juice49 requested a review from a team as a code owner September 30, 2024 14:20
@juice49 juice49 requested review from cngonzalez and removed request for a team September 30, 2024 14:20
@bjoerge bjoerge self-requested a review September 30, 2024 16:52
bjoerge
bjoerge previously approved these changes Sep 30, 2024
Copy link
Member

@bjoerge bjoerge left a comment

Choose a reason for hiding this comment

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

A great example of not only fixing the bug, but also making the code cleaner and removing complexity! Thank you so much for this @juice49. Left a few comments, no blockers for merge.

@juice49 juice49 merged commit df0e18a into next Oct 1, 2024
50 checks passed
@juice49 juice49 deleted the fix/sdx-1579/image-input-layout-shift branch October 1, 2024 10:48
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.

3 participants