[KZN-3415] uses native popover api in SingleSelect #5885

Kitty-Al · 2025-07-02T05:19:47Z

https://cultureamp.atlassian.net/browse/KZN-3415

What

Implements the native popover api which handles moving the popover into the css layer
~~uses useOverlayPosition to handle the popover positioning instead of using css anchor and having to manually handle the resizing & flipping of the popover~~
positions popover via css anchor positioning, if anchor positioning not supported we're now positioning manually via javascript

Positioning supports:

SSR
RTL layouts
Window resizing

Notes: Because we now handle the popover visibility manually via the Select component state we have to wire up the selected state, and clone the children passing the appropriate props.

Why

Reducing any future risk of z-index and layout issues. These were a regular occurance in the past

Initially positioning fallback was meant to be done with RAC - however RAC was not working in RTL layouts due to quirks of positioning in the css top-layer that RAC doesn't account for.

changeset-bot · 2025-07-02T05:19:51Z

⚠️ No Changeset found

Latest commit: 239f351

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2025-07-03T00:31:50Z

✨ Here is your branch preview! ✨

Storybook

Last updated for commit 239f351: Merge branch 'main' into KZN-3415/popover-api

mcwinter07 · 2025-07-04T02:04:10Z

packages/components/src/__alpha__/SingleSelect/SingleSelect.tsx

+        onSelectionChange={(key) =>
+          handleOnSelectionChange(key != null ? new Set([key]) : new Set())
+        }
+        className={classNameOverride}


One this I noticed was that we're using classNameOverride here instead of className. While this was the pattern for our older components, for recent ones like Button, Link and LinkButton we've allowed for className to be used so folks can leverage the RACs render props (see RAC styling section).

Looking at some of the sub-components that use RAC like ListItem they have className available so was just wondering if we are selectively choosing which ones have access to this?

Thanks for calling this out 😄 I never remember whether classNameOverride is new or old pattern given that it's still being used in some of our next components. I've removed it now and written myself a note 🧠

mcwinter07 · 2025-07-08T00:41:45Z

packages/components/src/__alpha__/SingleSelect/subcomponents/Trigger/Trigger.tsx

+interface TriggerProps {
+  buttonRef: React.RefObject<HTMLButtonElement>
+}
+


I noticed that were using an interface for TriggerProps and was wandering if there's a specifc reason for chosing this over type?

In the past we made a decision to lean on type over interface unless we foresaw the need to extend / merge a type. The main reasoning for where we landed was to simplify the decision process of whether something should be an interface or type, which has lead to most of our codebase using type.

mcwinter07 · 2025-07-08T01:03:51Z

packages/components/src/__alpha__/SingleSelect/types.ts

@@ -0,0 +1,9 @@
+export type SelectItem = {
+  label: string


Question on the types here, is the item always going to have a string label or is will it need to be a React node / element to be able handle Avatars, icons and such?

This is definitely not set in stone, and will most likely change. My thoughts are that we would support a Key: number or string. Passing in items will be mostly used for handling state and async behaviour. If users want to render a bespoke item they can do so by mapping over their own ListItem and passing it as children.

mcwinter07 · 2025-07-08T01:10:27Z

packages/components/src/__alpha__/SingleSelect/_docs/SingleSelect.stories.tsx

@@ -12,7 +13,9 @@ export default meta
 type Story = StoryObj<typeof meta>

 export const Playground: Story = {
-  args: {},
+  args: {
+    items: singleMockItems,


I noticed in the playground that when there is no children there appears to be a bullet rendered.

I'm assume we'll tackle what null or an empty children prop looks like later but just wanted to flag it in this story :)

mcwinter07 · 2025-07-08T01:22:47Z

packages/components/src/__alpha__/SingleSelect/subcomponents/Popover/Popover.tsx

+          zIndex: 'none',
+          margin: '0',
+          boxSizing: 'border-box',
+          width: buttonRef.current?.getBoundingClientRect().width ?? '200px',


So this would mean whatever the trigger button size is there popover box will be. Is there plans to give the consumers the ability to control the trigger styles a little more, ie: either a custom trigger button or giving them a className prop?

At the moment these styles are still temporary, as we don't have the designs for the popover yet.

mcwinter07 · 2025-07-08T01:26:59Z

packages/components/src/__alpha__/SingleSelect/SingleSelect.tsx

+        placeholder=""
+        {...restProps}
+      >
+        <Trigger buttonRef={buttonRef} />


The trigger at the moment is controlled entirely by our implementation with only the buttonRef exposed. Have we considered any potential needs for our consumers to either have a custom trigger or to be able to pass in data attributes that they may used in testing or for programmatically shifting focus?

The main thing that gets me thinking about that is this recent support question

Thanks for the callout. I totally appreciate that our users might need more flexibility here. I've written a ticket so that we capture the work on exposing the trigger & popover, trying to give our users as much flexibility as possible https://cultureamp.atlassian.net/browse/KZN-3462

mcwinter07 · 2025-07-08T01:32:56Z

packages/components/src/__alpha__/SingleSelect/SingleSelect.spec.tsx

  describe('renders', () => {
    it('a basic select component', () => {
      const { getByRole } = render(<SingleSelectWrapper />)
      const select = getByRole('button')
      expect(select).toBeInTheDocument()
    })
  })
+
+  describe('popover interactions', () => {


I keen to get thoughts about potentially moving the interaction tests over to Storybook's play tests 🤔

I don't think we have a hard and fast rule about when to use one over the other so might also be worth discussing as a team when we're all back :)

mcwinter07 · 2025-07-08T01:49:21Z

packages/components/src/__alpha__/SingleSelect/subcomponents/Popover/Popover.tsx

+    shouldFlip: true,
+  })
+  return (
+    <RACPopover trigger="manual" isOpen={isOpen} onOpenChange={setOpen} ref={racPopoverRef}>


I think we'll need to give this an aria-labelledby to ensure the element with the role dialog has a title

mcwinter07

One thing we might also need to consider is the keyboard behaviour for arrow keys to open the popover when focused on the trigger. We received this feedback in the recent Intopia audit that elements with aria-haspopup="listbox" imply that arrow keys can be used to open dialogs.

…nt container

packages/components/src/__alpha__/SingleSelect/SingleSelect.tsx

packages/components/src/__alpha__/SingleSelect/_docs/SingleSelect.stories.tsx

packages/components/src/__alpha__/SingleSelect/subcomponents/Popover/Popover.tsx

packages/components/src/__alpha__/SingleSelect/utils/useFixedOverlayPosition.ts

packages/components/src/__alpha__/SingleSelect/subcomponents/Popover/Popover.tsx

packages/components/src/__alpha__/SingleSelect/utils/useFixedOverlayPosition.ts

packages/components/src/__alpha__/SingleSelect/subcomponents/Popover/Popover.tsx

ryanseddon · 2025-07-18T05:46:08Z

packages/components/src/__alpha__/SingleSelect/subcomponents/Popover/Popover.tsx

-    return { '--position-anchor': anchorName } as React.CSSProperties
-  }
+    return getAnchorPositioningStyles(anchorName, positionData)
+  }, [hasAnchorSupport, anchorName, positionData])


I would invert this function and check if hasAnchorSupport is true and fallback to manual positioning:

if(!!hasAnchorSupport) { return getAnchorPositioningStyles(anchorName, positionData) } return getManualPositioningStyles(positionData)

ryanseddon · 2025-07-18T05:53:25Z

packages/components/src/__alpha__/SingleSelect/utils/useFixedOverlayPosition.ts

  useEffect(() => {
-    if (typeof window === 'undefined') return // SSR safety
+    // This effect only runs on the client after hydration
+    if (typeof window === 'undefined') return


You'd definitely want to useLayoutEffect here as this will run before painting and I would go back to the client hook but make it check if window/document exists and just bail e.g.

function useClientLayoutEffect( ...args) { if (typeof document === "undefined") return; useLayoutEffect(...args); }

This way if this gets SSR'd then it'll just not run anything and the name is clear that its client side only

ryanseddon · 2025-07-20T23:57:16Z

packages/components/src/__alpha__/SingleSelect/utils/useFixedOverlayPosition.ts

+/**
+ * Hook to calculate and update the position of an overlay rendered in the top layer.
+ */
+export function useFixedOverlayPosition({


We can simplify this in the case where the browser supports anchor positioning and we would only need to keep track of maxHeight adjustments otherwise anchor positioning can do everything else.

This is the CSS snippet I have on my local, maybe we pair on this.

@layer kz-components { .popover { // ... @supports (anchor-name: --anchor) { position-area: bottom center; position-try-fallbacks: flip-block; margin-block-start: 4px; width: anchor-size(width); } } }

[KZN-3415] uses native popover api in SingleSelect

a985a19

Kitty-Al added 6 commits July 2, 2025 15:28

[KZN-3415] fixes lint, ts, failing tests

c862422

[KZN-3415] adds styling to see a11y behaviour

7b4beed

[KZN-3415] adds tests for the popover

566129a

[KZN-3415] fix lint issues

6f62ad1

Merge branch 'main' into KZN-3415/popover-api

401c338

[KZN-3415] adds a note on popover in the docs

5632923

Kitty-Al marked this pull request as ready for review July 3, 2025 00:28

Kitty-Al added 2 commits July 3, 2025 15:06

Merge branch 'main' into KZN-3415/popover-api

6f84289

Merge branch 'main' into KZN-3415/popover-api

6b2cb5b

mcwinter07 reviewed Jul 4, 2025

View reviewed changes

[KZN-3415] removes classNameOverride

82f8f8e

mcwinter07 reviewed Jul 8, 2025

View reviewed changes

Kitty-Al added 6 commits July 8, 2025 13:00

[KZN-3415] change trigger to type

2744914

[KZN-3415] updates positioning logic

11f606b

[KZN-3415] moves interaction tests to storybook

c80d7d1

[KZN-3415] guards against SSR

d32fd52

Merge branch 'main' into KZN-3415/popover-api

c3c8afb

[KZN-3415] improves positioning function performance and handles pare…

428052a

…nt container

ryanseddon reviewed Jul 15, 2025

View reviewed changes

Kitty-Al added 2 commits July 15, 2025 12:00

[KZN-3415] removes popover toggle

40cc648

[KZN-3415] updates popover to handle flicker

0808407

ryanseddon reviewed Jul 15, 2025

View reviewed changes

packages/components/src/__alpha__/SingleSelect/subcomponents/Popover/Popover.tsx Outdated Show resolved Hide resolved

Kitty-Al added 3 commits July 15, 2025 12:06

Merge branch 'main' into KZN-3415/popover-api

738f5f1

[KZN-3415] WIP: using css anchor position

7788311

[KZN-3415] gives the popover a unique id

6f973d3

Kitty-Al added 5 commits July 17, 2025 16:28

[KZN-3415] refactor the popover positioning

d1b90fb

[KZN-3415] css anchoring position

06dd126

[KZN-3415] fixes lint issues

dc10e64

Merge branch 'main' into KZN-3415/popover-api

bf6918a

Merge branch 'main' into KZN-3415/popover-api

f0ab4e7

ryanseddon reviewed Jul 20, 2025

View reviewed changes

Merge branch 'main' into KZN-3415/popover-api

239f351

[KZN-3415] uses native popover api in SingleSelect #5885

Are you sure you want to change the base?

[KZN-3415] uses native popover api in SingleSelect #5885

Uh oh!

Conversation

Kitty-Al commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Uh oh!

changeset-bot bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcwinter07 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kitty-Al commented Jul 2, 2025 •

edited

Loading

changeset-bot bot commented Jul 2, 2025 •

edited

Loading

github-actions bot commented Jul 3, 2025 •

edited

Loading