Skip to content

Conversation

@mrcthms
Copy link
Contributor

@mrcthms mrcthms commented Oct 12, 2022

WHY are these changes introduced?

Fixes https://github.com/Shopify/web/issues/74392

Updates the loading state of the IndexTable to be a pulsing effect across all rows. The spinner for loading is now being handled as part of the filtering UI

Here's how it looks in a spinstance: https://admin.web.table-loading.marc-thomas.eu.spin.dev/store/shop1/orders?inContextTimeframe=none&delivery_method=

WHAT is this pull request doing?

Screen.Recording.2022-10-13.at.14.16.22.mov

🎩 checklist

@mrcthms mrcthms marked this pull request as ready for review October 13, 2022 11:27
@mrcthms
Copy link
Contributor Author

mrcthms commented Oct 13, 2022

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2022

size-limit report 📦

Path Size
polaris-react-cjs 209.07 KB (-0.03% 🔽)
polaris-react-esm 135.56 KB (-0.03% 🔽)
polaris-react-esnext 191.12 KB (+0.09% 🔺)
polaris-react-css 41.68 KB (+0.39% 🔺)

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221013112928
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221013112928
yarn add @shopify/polaris@0.0.0-snapshot-release-20221013112928

@mrcthms
Copy link
Contributor Author

mrcthms commented Oct 13, 2022

/snapit

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221013122010
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221013122010
yarn add @shopify/polaris@0.0.0-snapshot-release-20221013122010

@mrcthms
Copy link
Contributor Author

mrcthms commented Oct 13, 2022

/snapit

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221013123047
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221013123047
yarn add @shopify/polaris@0.0.0-snapshot-release-20221013123047

@mrcthms
Copy link
Contributor Author

mrcthms commented Oct 13, 2022

/snapit

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221013124431
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221013124431
yarn add @shopify/polaris@0.0.0-snapshot-release-20221013124431

Copy link
Member

@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.

A few concerns with this approach:

  1. Screen reader users. We need a way to clearly convey that loading is happening and some actions might be blocked
  2. Low vision users. There's not a lot of contrast going on here and the animation doesn't tell me that loading is happening anyway
  3. Performance. We used to have a shimmer on skeleton components that worked in a similar fashion. This was causing I think GPU or CPU issues on some machines. @GoodForOneFare do you remember any of the details around that issue? I seem to remember the results you got weren't from a mac or you were using a different tool to measure the processors

@GoodForOneFare
Copy link
Member

GoodForOneFare commented Oct 13, 2022

The old skeleton shimmer animation would overload CPU on Shopify Macbooks. The removal PR suggests "Slight performance problems", but multiple Macbooks recorded >400% CPU usage just from the animation.

@colinbendell looked into accelerating the animation, but others were adamant about removing it regardless of performance improvements.

I used MacOS's Activity Monitor tool to record CPU history. Launching that looks like this:

The CPU history looks like this:

@mrcthms
Copy link
Contributor Author

mrcthms commented Oct 13, 2022

/snapit

1 similar comment
@mrcthms
Copy link
Contributor Author

mrcthms commented Oct 13, 2022

/snapit

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221013162357
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221013162357
yarn add @shopify/polaris@0.0.0-snapshot-release-20221013162357

@github-actions
Copy link
Contributor

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

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

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221013162911
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221013162911
yarn add @shopify/polaris@0.0.0-snapshot-release-20221013162911

@mrcthms mrcthms closed this Feb 14, 2023
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