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

[EmptyState/Image] Fix image loading with ref #11874

Merged
merged 3 commits into from
Apr 10, 2024
Merged

Conversation

laurkim
Copy link
Contributor

@laurkim laurkim commented Apr 10, 2024

WHY are these changes introduced?

Resolves #11856.

EmptyState styles for loaded image were

WHAT is this pull request doing?

Refactors original logic from #11804 to use an HTMLImageElement ref and the complete attribute to set loaded image styles.

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

How to 🎩

Storybook
Spin

  • 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
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@laurkim laurkim force-pushed the lo/fix-empty-state-2 branch 4 times, most recently from 8e70831 to 2d8b5df Compare April 10, 2024 15:19
@laurkim

This comment was marked as outdated.

This comment was marked as outdated.

@laurkim laurkim marked this pull request as ready for review April 10, 2024 17:53
@laurkim laurkim self-assigned this Apr 10, 2024
@laurkim
Copy link
Contributor Author

laurkim commented Apr 10, 2024

@panzerdp feel free to test the snapshot for the changes to EmptyState in your app (context). When testing in a Remix sandbox and NextJS env, issues with loading state not being updated and styles for loading classes were resolved.

@panzerdp
Copy link

@laurkim Thank you so much for the fix!

I confirm that now the component works well during hydration, during regular client side rendering, and as well the CLS has been resolved.

@laurkim
Copy link
Contributor Author

laurkim commented Apr 10, 2024

/snapit

Copy link
Contributor

🫰✨ Thanks @laurkim! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240410194132"

@laurkim laurkim merged commit 7440367 into main Apr 10, 2024
13 checks passed
@laurkim laurkim deleted the lo/fix-empty-state-2 branch April 10, 2024 19:48
@laurkim laurkim mentioned this pull request Apr 11, 2024
6 tasks
sophschneider pushed a commit that referenced this pull request Apr 17, 2024
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

- [#11883](#11883)
[`a60d8aa4f`](a60d8aa)
Thanks [@chloerice](https://github.com/chloerice)! - Added a
`disclosureZIndexOverride` prop to `Filters`, `IndexFilters`, and `Tabs`
that is passed to `Popover` and `Tooltip` when provided


- [#11826](#11826)
[`a7fd7ab5d`](a7fd7ab)
Thanks [@sophschneider](https://github.com/sophschneider)! - Added
`contextualSaveBarVisible` and `contextualSaveBarProps` to `Frame`
context

### Patch Changes

- [#11842](#11842)
[`2a93578af`](2a93578)
Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed layout
shift for option lists within popovers


- [#11846](#11846)
[`ce6353b97`](ce6353b)
Thanks [@sophschneider](https://github.com/sophschneider)! - Restyled
Frame content behind dynamicTopBarAndReframe feature flag


- [#11872](#11872)
[`696bcb725`](696bcb7)
Thanks [@mattkubej](https://github.com/mattkubej)! - globally remove
link tap highlighting


- [#11874](#11874)
[`744036706`](7440367)
Thanks [@laurkim](https://github.com/laurkim)! - Added support for ref
to `Image` to handle image load with `EmptyState`


- [#11881](#11881)
[`c96ff56a0`](c96ff56)
Thanks [@sophschneider](https://github.com/sophschneider)! - Fixed Frame
feature override class to get proper max-width for main content.


- [#11885](#11885)
[`af80d3a82`](af80d3a)
Thanks [@craigcolesshopify](https://github.com/craigcolesshopify)! -
[indexTable] Fixed over scroll gap on `IndexTable` for sortable last
headings with `alignment="end"`


- [#11889](#11889)
[`374030428`](3740304)
Thanks [@chloerice](https://github.com/chloerice)! - Fixed `TextField`
zoom on focus due to font-size below 16px


- [#11900](#11900)
[`215b79271`](215b792)
Thanks [@sophschneider](https://github.com/sophschneider)! - Fixed
`Frame` scrollbar safe area to accommodate sidebar


- [#11842](#11842)
[`2a93578af`](2a93578)
Thanks [@kyledurand](https://github.com/kyledurand)! - Changed selected
icon position in Listbox and OptionList


- [#11891](#11891)
[`c84d4e875`](c84d4e8)
Thanks [@sophschneider](https://github.com/sophschneider)! - Moved
`Frame` scrollbar from main to content and set overflow-y from scroll to
auto behind a feature flag

## [email protected]

### Patch Changes

- Updated dependencies
\[[`a60d8aa4f`](a60d8aa),
[`2a93578af`](2a93578),
[`ce6353b97`](ce6353b),
[`696bcb725`](696bcb7),
[`744036706`](7440367),
[`c96ff56a0`](c96ff56),
[`af80d3a82`](af80d3a),
[`374030428`](3740304),
[`215b79271`](215b792),
[`a7fd7ab5d`](a7fd7ab),
[`2a93578af`](2a93578),
[`c84d4e875`](c84d4e8)]:
    -   @shopify/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
### WHY are these changes introduced?

Resolves Shopify#11856.

EmptyState styles for loaded image were

### WHAT is this pull request doing?

Refactors original logic from
[Shopify#11804](Shopify#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]>
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.

[EmptyStateExampleEmptyStateDefault]
3 participants