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

chore: Add collection example #977

Merged
merged 4 commits into from
Jan 12, 2022
Merged

chore: Add collection example #977

merged 4 commits into from
Jan 12, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Jan 10, 2022

Part of #939

Adds the default example for the Collection component and a simple test

@ciampo ciampo requested a review from diegohaz January 10, 2022 21:26
@ciampo ciampo self-assigned this Jan 10, 2022
@vercel
Copy link

vercel bot commented Jan 10, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

ariakit – ./

🔍 Inspect: https://vercel.com/ariakit/ariakit/8eHUtutcqAhH3XZMQttdLi7yXNRD
✅ Preview: https://ariakit-git-add-example-collection-default-ariakit.vercel.app

reakit – ./

🔍 Inspect: https://vercel.com/ariakit/reakit/J8Jz17jyM2K16s5CjtxKR2wGgVw3
✅ Preview: Canceled

[Deployment for a379ef1 canceled]

@ciampo ciampo mentioned this pull request Jan 10, 2022
69 tasks
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 10, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #977 (a379ef1) into v2 (8ec7290) will increase coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v2     #977      +/-   ##
==========================================
+ Coverage   64.25%   64.58%   +0.33%     
==========================================
  Files         170      171       +1     
  Lines        4233     4236       +3     
  Branches     1167     1167              
==========================================
+ Hits         2720     2736      +16     
+ Misses       1512     1499      -13     
  Partials        1        1              
Impacted Files Coverage Δ
...t/src/collection/__examples__/collection/index.tsx 100.00% <100.00%> (ø)
packages/ariakit/src/collection/collection-item.ts 100.00% <0.00%> (+29.41%) ⬆️
packages/ariakit/src/collection/collection.tsx 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ec7290...a379ef1. Read the comment docs.

@ciampo
Copy link
Contributor Author

ciampo commented Jan 10, 2022

@diegohaz let me know if the example is in line with the visual styles and the contents that have been used throughout the project.

For the test, I couldn't really think of anything in particular to test for, since the Collection component by default renders a few divs

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

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

Thank you @ciampo!

The main use for the Collection components is to keep the state in sync with a collection of DOM elements. For example, given the following elements:

const collection = useCollectionState();
<Collection state={collection}>
  <CollectionItem>Apple</CollectionItem>
  <CollectionItem>Grape</CollectionItem>
  <CollectionItem>Orange</CollectionItem>
</Collection>

I should be able to access the elements refs through the collection state:

collection.items.forEach((item) => {
  console.log(item.ref.current);
});

But it's really useful in combination with the getItem prop, which allows us to register any prop to the state besides the element ref:

function ToolsPanelItem({ value, isDisabled, ...props }) {
  const getItem = React.useCallback(
    (defaultItem) => ({ ...defaultItem, value, isDisabled }),
    [value, isDisabled]
  );
  return <CollectionItem getItem={getItem} hidden={isDisabled} {...props} />
}

function Panel() {
  const collection = useCollectionState();
  <Collection state={collection}>
    <ToolsPanelItem value="Apple">Apple</ToolsPanelItem>
    <ToolsPanelItem value="Grape" isDisabled>Grape</ToolsPanelItem>
    <ToolsPanelItem value="Orange">Orange</ToolsPanelItem>
  </Collection>
}

We can now access value and isDisabled through the state:

collection.items.map((item) => (
  <MenuItemCheckbox
    key={item.value}
    checked={!isDisabled}
    value={item.value}
  >
    {item.value}
  </MenuItemCheckbox>
));

It also keeps the order of the elements in sync with the DOM, so rendering elements dynamically will not push them to the end of the items list in the state:

<CollectionItem>Apple</CollectionItem>
{isAdmin && <CollectionItem>Grape</CollectionItem>}
<CollectionItem>Orange</CollectionItem>

In the example above, Grape will be the second element even if it's lazily rendered.

As a more concrete use case, this could be used here to keep the menu items in sync with the actual panel tools:

Opening a menu with items that reflect a collection of elements in the DOM


That being said, that panel is a really complex example, and I don't think we should have something like that as the default example. Even the getItem prop should probably be another example. But maybe a simple way to exemplify the collection features is to render a count.

@diegohaz diegohaz changed the title chore: add Collection default example chore: Add collection example Jan 11, 2022
@ciampo
Copy link
Contributor Author

ciampo commented Jan 11, 2022

Thank you @diegohaz for the review — I addressed all the feedback so far.

That being said, that panel is a really complex example, and I don't think we should have something like that as the default example. Even the getItem prop should probably be another example. But maybe a simple way to exemplify the collection features is to render a count.

Yeah, that example is quite complex and I'm not sure I get exactly how, in your example, MenuItemCheckbox is able to write the collection state — but I'm sure it's going to be more clear when that example gets added to the codebase.

The main use for the Collection components is to keep the state in sync with a collection of DOM elements. For example, given the following elements:
...
But it's really useful in combination with the getItem prop, which allows us to register any prop to the state besides the element ref:

All of this insight is great, and I think it would benefit the consumers of the component is it was added to its README. Right now the documentation for the component is not very clear about what this component is useful for and its main features.

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you!

@diegohaz
Copy link
Member

Yeah, that example is quite complex and I'm not sure I get exactly how, in your example, MenuItemCheckbox is able to write the collection state — but I'm sure it's going to be more clear when that example gets added to the codebase.

Yeah! That was just something I made off the top of my head. But here's a quick prototype of how that could work (https://codesandbox.io/s/ariakit-toolspanel-nf8xf?file=/App.tsx). Basically, CollectionItem would control the availability of each tool, whereas the menu state would control the visibility.

All of this insight is great, and I think it would benefit the consumers of the component is it was added to its README. Right now the documentation for the component is not very clear about what this component is useful for and its main features.

Agree! We should definitely include an explanation in the docs.

@diegohaz diegohaz merged commit a1fdc60 into v2 Jan 12, 2022
@diegohaz diegohaz deleted the add/example-collection-default branch January 12, 2022 02:07
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.

None yet

2 participants