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.

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

[EmptyState] Fix layout shift when image is loading with skeleton image #11804

Merged
merged 14 commits into from
Apr 1, 2024
Merged

[EmptyState] Fix layout shift when image is loading with skeleton image #11804

merged 14 commits into from
Apr 1, 2024

Conversation

laurkim
Copy link
Contributor

@laurkim laurkim commented Mar 29, 2024 •
edited
Loading

WHY are these changes introduced?

Resolves #1545.

Fixes layout shift when image is loading in EmptyState.

WHAT is this pull request doing?

Initially renders a skeleton image with set width/height to prevent layout shift as image asset is loading.
Renders final <Image> component with transition to prevent flicker for smoother UX.

EmptyState — before EmptyState — before
EmptyState — after EmptyState — after

How to 🎩

Spin

  • Throttle network (open dev console, select Network tab, choose either Fast 3G or Slow 3G)
  • Refresh page that renders EmptyState

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@laurkim laurkim added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Mar 29, 2024
@laurkim laurkim removed the 🤖Skip Changelog Causes CI to ignore changelog update check. label Mar 29, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@laurkim laurkim changed the title [EmptyState] Update to check if image is loaded [EmptyState] Set height to prevent layout shift Mar 29, 2024
@laurkim laurkim changed the title [EmptyState] Set height to prevent layout shift [EmptyState] Render image placeholder under loaded Mar 29, 2024
@laurkim laurkim changed the title [EmptyState] Render image placeholder under loaded [EmptyState] Render image placeholder until loaded Mar 29, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@laurkim laurkim changed the title [EmptyState] Render image placeholder until loaded [EmptyState] Add hook to render image placeholder until image loaded Mar 29, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@laurkim laurkim changed the title [EmptyState] Add hook to render image placeholder until image loaded [EmptyState] Fix layout shift when image is loading with skeleton image Mar 29, 2024
@laurkim laurkim marked this pull request as ready for review March 29, 2024 20:59
@laurkim laurkim self-assigned this Mar 29, 2024
@laurkim laurkim added the Bug Something is broken and not working as intended in the system. label Mar 29, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@laurkim laurkim requested a review from kyledurand April 1, 2024 13:21
kyledurand
kyledurand approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

👏

sophschneider
sophschneider approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@sophschneider sophschneider left a comment

Choose a reason for hiding this comment

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

Beauty! Thanks for doing this <3

polaris-react/src/components/EmptyState/EmptyState.module.css Outdated Show resolved Hide resolved
@laurkim
Copy link
Contributor Author

laurkim commented Apr 1, 2024

/snapit

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 1, 2024

🫰✨ Thanks @laurkim! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/[email protected]
yarn add @shopify/[email protected]
yarn add @shopify/[email protected]

sophschneider
sophschneider approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@sophschneider sophschneider left a comment

Choose a reason for hiding this comment

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

💫

@laurkim laurkim merged commit d1b46c2 into main Apr 1, 2024
14 checks passed
@laurkim laurkim deleted the lo/fix-empty-state branch April 1, 2024 14:20
@shopify-github-actions-access shopify-github-actions-access bot mentioned this pull request Apr 1, 2024
Merged
sam-b-rose pushed a commit that referenced this pull request Apr 1, 2024
@shopify-github-actions-access @github-actions
Version Packages (#11793)
e8fda67 
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/[email protected]

### Minor Changes

- [#11547](#11547)
[`df5276317`](df52763)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Applied semantic
type styles using the `Text` component


- [#11728](#11728)
[`281c8f8e9`](281c8f8)
Thanks [@kyledurand](https://github.com/kyledurand)! - Added new
AlphaPicker component


- [#11645](#11645)
[`b726dadbb`](b726dad)
Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Added
`useCopyToClipboard` hook for building copy actions matching common
actions guidelines


- [#11780](#11780)
[`4fffc2dcc`](4fffc2d)
Thanks [@itwasmattgregg](https://github.com/itwasmattgregg)! - allows
icons to be displayed on primary actions on Page component


- [#11547](#11547)
[`df5276317`](df52763)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Added
`base`,`inherit`, `disabled`, and `text-inverse` tone options for Text
component


- [#11547](#11547)
[`df5276317`](df52763)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Updated
plain/monochrome Button text size to bodySm for micro

### Patch Changes

- [#11789](#11789)
[`36df1aa6c`](36df1aa)
Thanks [@laurkim](https://github.com/laurkim)! - Fixed logo spacing on
`ContextualSaveBar`


- [#11794](#11794)
[`ffdcf1df7`](ffdcf1d)
Thanks [@kyledurand](https://github.com/kyledurand)! - Set default
scrollbar width to thin on scrollable


- [#11804](#11804)
[`d1b46c25c`](d1b46c2)
Thanks [@laurkim](https://github.com/laurkim)! - Fixed layout shift on
`EmptyState` when image is loading with skeleton image

## @shopify/[email protected]

### Minor Changes

- [#11547](#11547)
[`df5276317`](df52763)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Added warning for
`font-size`, `line-height`, and `font-weight` properties. Use the `Text`
component as a preferred option.

## @shopify/[email protected]

### Patch Changes

- Updated dependencies
\[[`df5276317`](df52763)]:
    -   @shopify/[email protected]

## [email protected]

### Minor Changes

- [#11779](#11779)
[`86a6ba44a`](86a6ba4)
Thanks [@itwasmattgregg](https://github.com/itwasmattgregg)! - Added
examples for `Card` and `Page` with icon actions

### Patch Changes

- [#11547](#11547)
[`df5276317`](df52763)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Added page for
`typography/property-disallow-list` Stylelint rule

- Updated dependencies
\[[`df5276317`](df52763),
[`281c8f8e9`](281c8f8),
[`b726dadbb`](b726dad),
[`4fffc2dcc`](4fffc2d),
[`df5276317`](df52763),
[`36df1aa6c`](36df1aa),
[`df5276317`](df52763),
[`ffdcf1df7`](ffdcf1d),
[`d1b46c25c`](d1b46c2)]:
    -   @shopify/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@panzerdp
Copy link

panzerdp commented Apr 8, 2024 •
edited
Loading

Unfortunately this change to <EmptyState /> creates a problem when using Remix and server-side rendering.

In my case, the empty state component is rendered on the server side and then sent to the client. On the server side there is no image loading process thus the component stays in the image not loaded state - and it stays the same way when hydrated on the client side.

Screenshot 2024-04-08 at 10 13 41

On a side note, an alternative solution is to simply indicate the image size directly, which would solve the CLS problem and also make it SSR compatible.

@laurkim laurkim mentioned this pull request Apr 10, 2024
Merged
6 tasks
laurkim added a commit that referenced this pull request Apr 10, 2024
@laurkim @kyledurand
[EmptyState/Image] Fix image loading with ref (#11874)
7440367 
### WHY are these changes introduced?

Resolves #11856.

EmptyState styles for loaded image were

### WHAT is this pull request doing?

Refactors original logic from
[#11804](#11804) to use an
`HTMLImageElement` ref and the [complete
attribute](https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/complete)
to set loaded image styles.
  <details>
    <summary>EmptyState — before</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/959a3bc9-d270-44fa-a916-6655532f82e5"
alt="EmptyState — before">
  </details>
  <details>
    <summary>EmptyState — after</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/9304f32d-a95b-41ea-add9-0e37a01a2704"
alt="EmptyState — after">
  </details>

### How to 🎩


[Storybook](https://5d559397bae39100201eedc1-oumakptmqa.chromatic.com/?path=/story/all-components-emptystate--default)

[Spin](https://admin.web.fix-empty-state-2.lo-kim.us.spin.dev/store/shop1/orders)

- For testing Spin instance, it helps to throttle the network to ensure
there's enough time to see the transition between placeholder and final
loaded asset

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

### 🎩 checklist

- [x] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [x] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide

---------

Co-authored-by: Kyle Durand <[email protected]>
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
@laurkim
[EmptyState] Fix layout shift when image is loading with skeleton ima…
8274e40 
…ge (Shopify#11804)

### WHY are these changes introduced?

Resolves
[Shopify#1545](https://github.com/Shopify/polaris-internal/issues/1545).

Fixes layout shift when image is loading in `EmptyState`.

### WHAT is this pull request doing?
Initially renders a skeleton image with set width/height to prevent
layout shift as image asset is loading.
Renders final `<Image>` component with transition to prevent flicker for
smoother UX.
  <details>
    <summary>EmptyState — before</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/d186b70e-7f59-40cb-b2ad-8487c1f8e276"
alt="EmptyState — before">
  </details>
  <details>
    <summary>EmptyState — after</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/31ad9f5f-80a9-4d8a-a325-9863458e292e"
alt="EmptyState — after">
  </details>

### How to 🎩


[Spin](https://admin.web.fix-empty-state.lo-kim.us.spin.dev/store/shop1/orders)
- Throttle network (open dev console, select `Network` tab, choose
either `Fast 3G` or `Slow 3G`)
- Refresh page that renders EmptyState

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

### 🎩 checklist

- [x] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyledurand kyledurand kyledurand approved these changes

@sophschneider sophschneider sophschneider approved these changes

@stewx stewx Awaiting requested review from stewx

@johanstromqvist johanstromqvist Awaiting requested review from johanstromqvist

Assignees

@laurkim laurkim

Labels
Bug Something is broken and not working as intended in the system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laurkim @panzerdp @kyledurand @sophschneider