Refactor collection internals in preparation for public API #6608
Conversation
| focusMode?: 'row' | 'cell' | ||
| focusMode?: 'row' | 'cell', | ||
| /** @private - do not use unless you know what you're doing. */ | ||
| UNSAFE_selectionState?: MultipleSelectionState |
There was a problem hiding this comment.
Don't love this, but we need to avoid a circular dependency in RAC Table. We need the selection state in order to determine whether to render checkboxes based on the selectionBehavior etc (via useTableOptions), but we can't build the full TableState until we have the collection. This prop lets us split this up to call useMultipleSelectionState directly, and pass this state down into useTableState. It is unsafe because of the rules of hooks below, so this is not meant to be a public API. Happy to change if someone has a better idea.
There was a problem hiding this comment.
Would it make more sense for useMultipleSelectionState to have this and reconcile the difference? then if we run into this issue in other places it's not just in useGridState?
Also, we could not call it conditionally below, it just sets up state and then if nothing interacts with it, it won't do anything. So should be harmless to call it unconditionally and only use the result if props.UNSAFE_selectionState isn't passed in.
There was a problem hiding this comment.
yeah maybe. It bothers me to do extra unnecessary work and throw it away lol
| return ( | ||
| <> | ||
| <Hidden> | ||
| <CollectionDocumentContext.Provider value={document}> |
There was a problem hiding this comment.
These are technically not required for collections that render collection children directly (like Menu), it's only needed for cases like Select and ComboBox that have intermediary elements. It shouldn't hurt anything but if there were performance issues with this we could theoretically optimize it (e.g. check if props.children is a <Collection> element).
|
## API Changes
unknown top level export { type: 'any' } |
There was a problem hiding this comment.
LGTM, RAC collection components still seem to work as expected. Just one comment to clarify that I understand what is being exposed and why. We should discuss some names for CollectionBuilder haha (CollectionInitializer? Dunno, having a hard time not using builder in the name)
There was a problem hiding this comment.
So just for book keeping, the extra Collection stuff we are planning on exposing from a new package is as follows:
- CollectionBuilder (with a new name to differentiate it from the existing CollectionBuilder stuff from old collections)
- createLeaf/BranchComponent (for creating collection elements)
- CollectionRendererContext (to access various info from Virtualizer in your branch/leaf components)
- DefaultCollectionRenderer (for CollectionRoot and CollectionBranch)
with those (and the already exposed Collection), hooks users can use new collections for their components
There was a problem hiding this comment.
Yeah that sounds right. Not sure what to do about BaseCollection which is used to subclass in TableCollection too.
In preparation for exposing RAC's collection implementation publicly, this refactors the internals with a safer and less error-prone API that exposes fewer implementation details. It exposes a
CollectionBuildercomponent (name tbd??) that takes care of rendering the collection in a hidden tree and exposing the built collection.This ensures that the order is always correct (the portal must be before the rendered content for SSR), and avoids exposing things like the internal document and other implementation details.
The
contentprop can be any React children, either direct collection elements (e.g.MenuItem) or other elements (in the case ofSelectandComboBox). A<Collection>rendered inside starts a collection root and registers items with the collection owned byCollectionBuilder. Many collection components accept collection children and theitemsprop directly as a shortcut, so these rendercontent={<Collection {...props} />}.This will move to a separate package (out of RAC) so it can be used by hook users next. Wanted to do this refactor first so it's easier to review.