AnchoredOverlay and ActionList fixes for SelectPanel #1209
Conversation
🦋 Changeset detectedLatest commit: 3857674 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/B2BdNxg7zwkThmfV5nvJLcy5iom3 |
| @@ -153,10 +147,11 @@ export function List(props: ListProps): JSX.Element { | |||
| let groups: (GroupProps | (Partial<GroupProps> & {renderItem?: typeof Item; renderGroup?: typeof Group}))[] = [] | |||
|
|
|||
| // Collect rendered `Item`s into `Group`s, avoiding excess iteration over the lists of `items` and `groupMetadata`: | |||
| const singleGroupId = useMemo(uniqueId, []) | |||
There was a problem hiding this comment.
To clarify, this is here (as opposed to preceding line 154) to ensure the same number of hooks is called on each render, i.e. to avoid conditionally calling useMemo as the number of Groups changes?
There was a problem hiding this comment.
Yep, that's exactly why. Hooks shouldn't be called inside loops or conditionals to ensure consistent execution order across renders.
These changes address a number of issues/missing features that were found during development of the
SelectPanelcomponent (currently being built in a separate project)Overlayheight/width should be able to be set through theAnchorOverlaycomponent. This allows you to have a fixed height/width on the overlay if list items are loaded asynchronously after the overlay opens.Itemdata to include anidfield, which could be astringornumber. We should allow these to be passed in onItemPropsand use them as thekeyprop when present. We also don't want to use them on the actualItemelement in the DOM as they will likely not be globally uniqueActionListwill create a single group wrapper if it is passed a list of items without any group details. This group was previously receiving auniqueIdfor itsgroupIdon every render. This caused react to remove the existing DOM elements and create all new ones for the entire list contents. Not ideal for perf or for focus management.AnchoredOverlaywas delaying the focus zone from activating until user interaction triggeredlistmode. This is a problem if the user changes focus by clicking into the list. There is no reason not to activate the focus zone as soon as the overlay is opened. Focus trap should still be deferred, but there are remaining issues around when that activates with user clicks (Activate focus trap when clicking inside AnchoredOverlay #1210)Merge checklist