Add the portalContainerId in Select Future release #4328
Conversation
🦋 Changeset detectedLatest commit: 27d11ba 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 |
mcwinter07
changed the title
mcwinter07
force-pushed
the
KDS-1969/expose-portal-on-select
branch
from
9d51653 to
1199280
Compare
mcwinter07
force-pushed
the
KDS-1969/expose-portal-on-select
branch
from
fef77c5 to
d7d234c
Compare
mcwinter07
changed the title
|
|
||
| <Canvas of={SelectStories.PortalContainer} /> | ||
|
|
||
| It is important to note that a `relative` position should be applied to container element that contains the portal, ie: |
There was a problem hiding this comment.
I'd say "not absolute" rather than "is relative", as it will explain it clearer without the need for an example since everything is relative by default.
There was a problem hiding this comment.
But the container needs a position: relative for the popover to determine its position document since it uses absolute. Otherwise the static positioning on most elements could risk the Popover spilling out of the confines of it container.
Relative on container:
Classless container:
There was a problem hiding this comment.
Huh very curious, but do we want it to cut off..?
There was a problem hiding this comment.
I wouldn't cut it off as a default, but if someone wants to cut it off, I'm sure they can figure it out :) I think we could probably leave the advice off 😄
There was a problem hiding this comment.
We can remove it, but my thinking here is that this is a visual shorthand of how the portal container can effect the popover, instead of it just being on the body.
I'm less beholden to having the advice around relative at the bottom since the example code can be seen in the canvas anyway.
|
|
||
| describe("Popover portal", () => { | ||
| it("has accessible trigger controls", async () => { | ||
| render(<SelectWrapper id="id--select" isOpen />) |
There was a problem hiding this comment.
No need to declare the id in these tests
There was a problem hiding this comment.
And they're outa here ⚾ 🌬️ Nice catch, some left overs of when I had some janky selectors in there
| /** | ||
| * Creates a portal for the Popover to the matching element id | ||
| */ | ||
| portalContainerId?: string |
There was a problem hiding this comment.
I think instead of confining it to an id, we should expose the same api that we've given the Popover, which accepts portalContainer as an element. This way, the consumer can use a ref if they wanted and do not require an id :)
There was a problem hiding this comment.
This was my suggestion to simplify the API for the most common use case
There was a problem hiding this comment.
I'm in two minds about this, but having just tested variations of consumer code, I suppose this will be easiest for the consumer since they don't need a useEffect. Happy to go this way then :)
Co-authored-by: Geoffrey Chong <[email protected]>
Why
This is blocking adoption of the Select component in KAIO. Consumers were having issues with the select not having an expose portal prop.
What
portalContainerIdprop in futureSelectPopoverprimitive