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

Thoughts on Collection API consistency #6596

Open
ArrayKnight opened this issue Jun 23, 2024 · 9 comments
Open

Thoughts on Collection API consistency #6596

ArrayKnight opened this issue Jun 23, 2024 · 9 comments

Comments

@ArrayKnight
Copy link

ArrayKnight commented Jun 23, 2024

Provide your feedback here.

I'm curious if it might be possible to make some changes to the way RACs that implement the Collection API are structured.

The current pattern for most RACs is to allow for render props children and composability with slots. However, RACs that implement the Collection API, buck this trend. Children functions are item iterators, internal state of the parent component is unavailable (except to className and style), and they include renderable props for other states, such as renderEmptyState

This makes certain aspects of their internal state completely inaccessible unless the parent completely reimplements and controls that state.

🔦 Context

One of the easiest ways to gain access to the complete internal state of an RAC (without reimplementing and controlling its state from a parent) is to utilize the render props children and create an inner component that creates a custom rendering based on that state. But this isn't possible with RACs like ListBox, TagList and others

💻 Code Sample

My thinking is something along the lines of:

<ListBox
  aria-label="Pick a Pokemon"
  items={list.items}
  selectionMode="single"
>
  <ListBoxItems>{(item) => <ListBoxItem id={item.name}>{item.name}</ListBoxItem>}</ListBoxItems>
  <EmptyState slot="empty">No items</EmptyState>
</ListBox>
<ListBox
  aria-label="Pick a Pokemon"
  items={list.items}
  selectionMode="single"
>
  {(renderProps) => <MyCustomListBoxInner {...renderProps} />}
</ListBox>

Where MyCustomListBoxInner is likely to reimplement the above ListBoxItems & EmptyState but may also include some custom logic based on the internal state of the ListBox

Version

1.2.1

What browsers are you seeing the problem on?

Other

If other, please specify

Not a browser issue

What operating system are you using?

Mac & Windows

@LFDanLu
Copy link
Member

LFDanLu commented Jun 24, 2024

My first thought here would be it would be a breaking change if we changed how things like empty state was provided to a collection component (or at least we'd have to deprecate renderEmptyState and support both ways of providing it). As for allowing the render function of the ListBox to have renderProps, is there a particular use case you are trying to achieve with it? The contents of the ListBox itself are pretty specific and rigid (aka it expects only sections and items) so it feels like it would mainly be used for styling based on the state of the listbox but the ListBoxItem's themselves have access to renderProps in their own render functions for this.

@ArrayKnight
Copy link
Author

ArrayKnight commented Jun 25, 2024

Yup, this would absolutely be a breaking change, but in the name of consistency, flexibility and extensibility.

And just to reiterate, this is not exclusive to the ListBox. It is applicable to all RAC that implement the Collection API.

The use case is being able to access the entire collection state at the top level. Which would allow for the creation of custom components that can maintain the flexibility of static and dynamic collection items without needing to reimplement and control state.

This would enable the custom component to establish functionality based on this collection state such as filtering, searching, custom layout, etc. Alternatively, the custom component could establish context slots for composition of this functionality.

MyCustomComponent (receives all props of below RAC plus any custom, passes them through)
AnyRACCollectionComponent
MyCustomComponentInner (receives render props from above RAC plus any custom from parent)

Here the Inner component now has full access to the RAC internal state and nothing had to be reimplemented. It can now choose to establish slots with a Provider or custom layout and functionality, etc. And MyCustomComponent retained all of the flexibility of the underlying RAC

@ArrayKnight
Copy link
Author

A more concrete example would be to implement a De/Select All button. This would contextually need to know the state of the whole collection, what is currently selected, and access to the selection manager to be able to call methods.

As things currently stand the only way to do this is to drop support for statically defined items, implement useControlledState and useListState, and control the RAC completely from the parent thereby also allowing for the state and selection manager to be exposed as described above

With the proposed changes, the entire complexity of reimplementing state and selection would be negated

@LFDanLu
Copy link
Member

LFDanLu commented Jun 25, 2024

I see, we've have historically suggested that users drop down to the hooks levels at this point but I see your point about wanting more flexibility here by making the Collection RAC components support more of a wrapper like api. I remember there were issues in the past (infinite rerenders/etc) when trying to provide the internal state object via render props for some of the collection components like ComboBox and Select, but those are a bit of a different case so I'm not sure if the same issues would arise. I'll bring this up with the team and see what opinions they have.

@LFDanLu
Copy link
Member

LFDanLu commented Jun 26, 2024

Talked with the team and maybe the work in #6608 can help enable this for you. The exported Collection stuff from that PR will allow you to follow the same pattern that ComboBox and Select have aka the ListBox creates the collection of items that Combobox/Select can then use in their own hooks. That sounds like the structure you want here where you are sharing the state between a parent component and the actual collection component that is generating the collection of items.

We are a bit wary of performing the original proposed changes since it is such a big breaking change and since we weren't sure it would actually work in the dynamic case due to the parent state needing the collection to be passed in which would be problematic if ListBoxItems was now responsible for setting up the collection via CollectionBuilder (aka it is now at a lower level than were the top level state is setup)

@ArrayKnight
Copy link
Author

I'll check out the PR you linked in detail tomorrow. In the meantime, my gut reaction to how to make the original proposal work would be to have <ListBoxItems> be required to implement a slot prop, such that <ListBox> would pass down a ref and be able to access state / children. Technically unnecessary if the <ListBoxItems>'s context is just implemented on the DEFAULT_SLOT. Just an untested idea:

<ListBox
  aria-label="Pick a Pokemon"
  items={list.items}
  selectionMode="single"
>
  <ListBoxItems slot="items">{(item) => <ListBoxItem id={item.name}>{item.name}</ListBoxItem>}</ListBoxItems>
  <EmptyState slot="empty">No items</EmptyState>
</ListBox>

<ListBox
  aria-label="Pick a Pokemon"
  selectionMode="single"
>
  <ListBoxItems slot="items">
    <ListBoxItem>Foo</ListBoxItem>
    <ListBoxItem>Bar</ListBoxItem>
  </ListBoxItems>
  <EmptyState slot="empty">No items</EmptyState>
</ListBox>

So, <ListBox, implements <Provider values={[[ListboxItemsContext, { ref: itemsRef }]]}>. And potentially instead of the ref attaching to the DOM node, it connects to an imperative handle that exposes access to it's collection, maybe?

@devongovett
Copy link
Member

devongovett commented Jun 27, 2024

The problem is that there's a circular dependency. In order to construct the state, you first need the collection. So we can't pass the state as a render prop because it depends on the children which would be returned by the render prop function rendering first.

@ArrayKnight
Copy link
Author

ArrayKnight commented Jun 27, 2024

True, the initial render would be guaranteed to have an empty collection in the case of static jsx options (items prop would fulfill per usual, immediate if static, on second render if asynchronous/dynamic) until it was able to retrieve that state. Maybe not perfect, but not unworkable, and actually consistent with the asynchronous dynamic items

@ArrayKnight
Copy link
Author

ArrayKnight commented Jun 27, 2024

Alternative approach: Support a third type of rendering combination

  • Approach 1: items is undefined, children are static jsx collection items
  • Approach 2: items is an array, children is an iterator function
  • Approach 3: items is an array, children is a new component <CollectionRenderProps>{(renderProps) => {...}}</CollectionRenderProps>; where renderProps represents the full render props state of the parent RAC, which would include the collection, selectionManager, and any other applicable state (already outlined in the RAC's style renderProps)

No breaking change, no circular dependency, full access to internal state. Win, win, win?

And it should be relatively easy to determine which approach is being utilized to change how the collection is initialized

The only downside is the lack of support for static jsx items combined with being able to access render props children

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

No branches or pull requests

3 participants