Fix PopoverAnchor not taking precedence over PopoverDisclosure when placing Popover

[bengry]

Fixes #3729

This is my first PR to this repository, so although I read the contributing file, I might've goofed or missed something. Please let me know if there's anything I should've done differently.

[changeset-bot]

🦋 Changeset detected

Latest commit: d6918e4

The changes in this PR will be included in the next version bump.

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

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[diegohaz]

Thanks for tackling this, @bengry!

I think this implementation might introduce a breaking change, as some folks could expect the anchorElement state in the popover store to reference the disclosure element.

As an alternative, we could continue to use the popover anchor logic within the disclosure component, but introduce a kind of internal priority prop. Here, the usePopoverDisclosure hook would have a lower priority compared to a component that directly uses usePopoverAnchor (like PopoverAnchor). This could involve passing an internal prop or something similar. I'm still pondering over this.

[bengry]

Thanks for tackling this, @bengry!

I think this implementation might introduce a breaking change, as some folks could expect the anchorElement state in the popover store to reference the disclosure element.

Is this documented behavior? I think the code would be much harder to reason about.

As an alternative, we could continue to use the popover anchor logic within the disclosure component, but introduce a kind of internal priority prop. Here, the usePopoverDisclosure hook would have a lower priority compared to a component that directly uses usePopoverAnchor (like PopoverAnchor). This could involve passing an internal prop or something similar. I'm still pondering over this.

I tried implementing this, but I'm not sure what's the proper way to pass in or save an internal state like this in Ariakit, especially because the library exposes all the interfaces and hooks as it's public API it makes things a bit tricky.
Maybe if you can point me in the right direction I could take it further.

Anyway, IMO, especially because the library is still in pre-v1, and a migration guide is added to the release notes (basically, use my workaround, but the other way around, elements-wise) is enough.

[bengry]

Anyway, IMO, especially because the library is still in pre-v1, and a migration guide is added to the release notes (basically, use my workaround, but the other way around, elements-wise) is enough.

@diegohaz Hey, any news on this? Would love to get this fix in (although not urgent since it does have a workaround, as you've mentioned).