[IndexTable]: optimizes and add more stuff to `use-index-resource-state` #11657

lbenie · 2024-02-27T16:09:34Z

WHY are these changes introduced?

Closes: https://github.com/Shopify/admin-comms/issues/1261

WHAT is this pull request doing?

This PR introduces 2 new states for the use-index-resource-state hook.

Dirty -- A boolean that reflects the dirty state of the IndexTable
UnselectedResources -- A string array of IDs. This array adds/removes IDs that were previously selected. It's initially set to an empty array.

Also, there were some performance improvements by using a Set instead of an Array. In a lot of cases we are concerned with the lookup access and using a Set gives us a O(1) access time compared to O(N) on the array since we were using operations like filter.

The hook's API returned object still exposes an array and not a Set.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

Tested a snapshot
Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

lbenie · 2024-02-27T16:10:43Z

polaris-react/src/utilities/tests/use-index-resource-state.test.tsx

-    );
-  }
-
-  function MockClearComponent<T extends {[key: string]: unknown}>({


this was unnecessary since we can do everything from the "main" component

lbenie · 2024-02-27T16:12:02Z

polaris-react/src/utilities/use-index-resource-state.ts

+  const [unselectedResources, setUnselectedResources] = useState<
+    ReadonlySet<string>
+  >(new Set());
+  const [dirty, setDirty] = useState(false);


This state is needed for Test drive.

We need to know if the user made some changes on the table and we display some text based on this condition

lbenie · 2024-02-27T16:13:18Z

polaris-react/src/utilities/use-index-resource-state.ts

  const [allResourcesSelected, setAllResourcesSelected] = useState(
    initAllResourcesSelected,
  );
+  const [unselectedResources, setUnselectedResources] = useState<


This state is needed for Test drive.

We need to know all the "stores" that were unselected. In our UX, we pre-check every store and if they become unselected we must update them on the backend.

lbenie · 2024-02-27T16:14:06Z

polaris-react/src/utilities/use-index-resource-state.ts

+  useEffect(() => {
+    if (!isEqual(prevPreCheckedResourcesRef.current, preCheckedResources)) {
+      const filteredResources = dirty
+        ? preCheckedResources.filter(
+            (resource) => !unselectedResources.has(resource),
+          )
+        : preCheckedResources;
+
+      setSelectedResources(new Set(filteredResources));
+      prevPreCheckedResourcesRef.current = preCheckedResources;
+    }
+  }, [dirty, preCheckedResources, unselectedResources]);


This makes sure that preCheckedResources are synced on pagination.

The current behavior does not re-hydrates on pagination since the state is never updated.

lbenie · 2024-02-27T16:14:37Z

polaris-react/src/utilities/use-index-resource-state.ts

+  useEffect(() => {
+    return () => {
+      setDirty(false);
+    };
+  }, []);


Make sure the state is cleaned when the component unmounts

lbenie · 2024-02-27T16:16:08Z

polaris-react/src/utilities/use-index-resource-state.ts

-              ? [...newSelectedResources, selection as string]
-              : newSelectedResources.filter((id) => id !== selection),
-          );
+          if (typeof selection !== 'string') break;


It felt odd to use selection as String when the Types allowed for more.

lbenie · 2024-02-27T19:18:08Z

/snapit

github-actions · 2024-02-27T19:21:18Z

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

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

yarn add @shopify/polaris-icons@0.0.0-snapshot-20240227192044

yarn add @shopify/polaris@0.0.0-snapshot-20240227192044

mrcthms

Code changes are looking good to me! Would love to get a Spin URL to test this snapshot with in the PR description before approving

lbenie · 2024-02-28T14:30:35Z

Code changes are looking good to me! Would love to get a Spin URL to test this snapshot with in the PR description before approving

Yes I spun an environment last night but kids prevented me from working haha, will get something up in a few!

@mrcthms

lbenie · 2024-02-28T14:57:36Z

Here's the Product Index

Here's Test drive

For test drive you need to click on manage so show the modal and see the Index Table.

One thing I've noticed is we are still bound to pass a bulk/promoted actions in order to show the paginated text.

mrcthms · 2024-02-28T16:02:15Z

@lbenie Created a PR for the paginated select all action text issue – #11670 My apologies on that one, thought we'd get that for free if we enabled the regular select actions in the bulk actions, but evidently not!

mrcthms · 2024-02-28T16:07:17Z

Also just want to confirm, taking a look at the two URLs there, I can discern no difference from the behaviour that we currently have in production – is that what we expect?

lbenie · 2024-02-28T16:16:59Z

Also just want to confirm, taking a look at the two URLs there, I can discern no difference from the behaviour that we currently have in production – is that what we expect?

Yes that was the goal, API is retro-compatible meaning that it shouldn't break any IndexTable using this hook.

Also it adds some perf/stuff to the API (dirty/unselected) so it's "opt-in" from a consumer perspective.

On a side note, I began investigating last night about this design.

Here's my plan to achieve this

We'd need to call the clearSelection callback from the use-index-resource-state. In order to achieve to we'd need to pass this as a prop to IndexTable (same way it's done for handleChange)
Not sure if we need to bind it to the context
If all is selected, call clear selection

Thoughts on ^

I think as a follow-up on web for test drive, I can check if unselected.length > 0 && all test drive are enabled then fully delete them with the API.

lbenie · 2024-02-28T18:35:04Z

/snapit

github-actions · 2024-02-28T18:38:35Z

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

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

yarn add @shopify/polaris@0.0.0-snapshot-20240228183806

lbenie · 2024-03-18T20:43:54Z

/snapit

github-actions · 2024-03-18T20:47:20Z

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-20240318204532

yarn add @shopify/polaris@0.0.0-snapshot-20240318204532

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-20240318204532

lbenie · 2024-03-19T18:23:55Z

Spin:

What to expect, no changes and regressions on products. For test drive, if you select/unselect it should send a different payload to the API with the proper resources.

@mrcthms @alex-page

Here's the diff with web: https://github.com/Shopify/web/compare/main...polaris-index-state

Can I get this merged for tomorrow?

lbenie · 2024-03-21T17:52:12Z

/snapit

github-actions · 2024-03-21T17:55:21Z

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-20240321175357

yarn add @shopify/polaris@0.0.0-snapshot-20240321175357

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-20240321175357

lbenie · 2024-03-21T18:13:33Z

/snapit

github-actions · 2024-03-21T18:16:57Z

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-20240321181526

yarn add @shopify/polaris@0.0.0-snapshot-20240321181526

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-20240321181526

lbenie · 2024-03-25T14:49:31Z

/snapit

github-actions · 2024-03-25T14:52:53Z

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

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

yarn add @shopify/polaris-migrator@0.0.0-snapshot-20240325145122

yarn add @shopify/polaris@0.0.0-snapshot-20240325145122

yarn add @shopify/stylelint-polaris@0.0.0-snapshot-20240325145122

chloerice

Hey @lbenie 👋🏽 Changes overall look good, but I noticed while tophatting that when SelectionType is Range (e.g., with subheaders, with nested rows) the "All" deselection breaks. 👀

lbenie · 2024-04-15T19:47:44Z

thanks @chloerice I'll investigate

translation-platform · 2024-04-19T21:55:01Z

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

lbenie self-assigned this Feb 27, 2024

lbenie changed the title ~~[IndexTable]: optimizes and add more stuff to~~ [IndexTable]: optimizes and add more stuff to use-index-resource-state Feb 27, 2024

lbenie commented Feb 27, 2024

View reviewed changes

lbenie requested review from mrcthms and alex-page February 27, 2024 19:17

lbenie marked this pull request as ready for review February 27, 2024 19:18

boyutf self-assigned this Feb 27, 2024

lbenie force-pushed the test-drive/use-index-resouce-state-fast-follow branch 2 times, most recently from 91df46b to e082aef Compare February 27, 2024 21:39

mrcthms reviewed Feb 28, 2024

View reviewed changes

lbenie force-pushed the test-drive/use-index-resouce-state-fast-follow branch 2 times, most recently from 839f0a7 to 3e69e73 Compare February 28, 2024 18:53

lbenie force-pushed the test-drive/use-index-resouce-state-fast-follow branch from 3e69e73 to 12e0ddb Compare March 18, 2024 20:42

[IndexTable]: optimizes and add more stuff to

b92a364

lbenie force-pushed the test-drive/use-index-resouce-state-fast-follow branch from 2968e00 to 82f1811 Compare March 19, 2024 18:21

alex-page requested review from a team and removed request for alex-page March 20, 2024 00:02

fix

e4eeca5

fix

a06494f

fix test

b029416

chloerice reviewed Apr 15, 2024

View reviewed changes

aaronccasanova force-pushed the test-drive/use-index-resouce-state-fast-follow branch from 7fe360a to b029416 Compare April 19, 2024 21:52

lbenie requested review from alex-page and sam-b-rose as code owners April 19, 2024 21:52

github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024

aaronccasanova force-pushed the main branch from 9f6fc03 to b71eff5 Compare April 20, 2024 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IndexTable]: optimizes and add more stuff to `use-index-resource-state` #11657

[IndexTable]: optimizes and add more stuff to `use-index-resource-state` #11657

lbenie commented Feb 27, 2024 •

edited

Loading

lbenie Feb 27, 2024

lbenie Feb 27, 2024

lbenie Feb 27, 2024

lbenie Feb 27, 2024

lbenie Feb 27, 2024

lbenie Feb 27, 2024

lbenie commented Feb 27, 2024

github-actions bot commented Feb 27, 2024

mrcthms left a comment

lbenie commented Feb 28, 2024

lbenie commented Feb 28, 2024

mrcthms commented Feb 28, 2024

mrcthms commented Feb 28, 2024

lbenie commented Feb 28, 2024 •

edited

Loading

lbenie commented Feb 28, 2024

github-actions bot commented Feb 28, 2024

lbenie commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

lbenie commented Mar 19, 2024

lbenie commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

lbenie commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

lbenie commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

chloerice left a comment

lbenie commented Apr 15, 2024

translation-platform bot commented Apr 19, 2024

[IndexTable]: optimizes and add more stuff to use-index-resource-state #11657

Are you sure you want to change the base?

[IndexTable]: optimizes and add more stuff to use-index-resource-state #11657

Conversation

lbenie commented Feb 27, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

lbenie Feb 27, 2024

Choose a reason for hiding this comment

lbenie Feb 27, 2024

Choose a reason for hiding this comment

lbenie Feb 27, 2024

Choose a reason for hiding this comment

lbenie Feb 27, 2024

Choose a reason for hiding this comment

lbenie Feb 27, 2024

Choose a reason for hiding this comment

lbenie Feb 27, 2024

Choose a reason for hiding this comment

lbenie commented Feb 27, 2024

github-actions bot commented Feb 27, 2024

mrcthms left a comment

Choose a reason for hiding this comment

lbenie commented Feb 28, 2024

lbenie commented Feb 28, 2024

mrcthms commented Feb 28, 2024

mrcthms commented Feb 28, 2024

lbenie commented Feb 28, 2024 • edited Loading

lbenie commented Feb 28, 2024

github-actions bot commented Feb 28, 2024

lbenie commented Mar 18, 2024

github-actions bot commented Mar 18, 2024

lbenie commented Mar 19, 2024

lbenie commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

lbenie commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

lbenie commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

chloerice left a comment

Choose a reason for hiding this comment

lbenie commented Apr 15, 2024

translation-platform bot commented Apr 19, 2024

[IndexTable]: optimizes and add more stuff to `use-index-resource-state` #11657

[IndexTable]: optimizes and add more stuff to `use-index-resource-state` #11657

lbenie commented Feb 27, 2024 •

edited

Loading

lbenie commented Feb 28, 2024 •

edited

Loading