Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Render <MainTreeNode /> indicators in Popover.Group only #2634

Merged
merged 4 commits into from
Aug 2, 2023
Merged

Render <MainTreeNode /> indicators in Popover.Group only #2634

merged 4 commits into from
Aug 2, 2023

Conversation

RobinMalfait
Copy link
Member

This PR improves the rendered DOM structure for Popover components.

Some of our components auto close when you click outside of them, the Popover component is one of them. However, there are a few exceptions to this rule:

  • If you render a notification on top of them and click on the × to close the notification then we don't want to close the currently open Popover.
  • If you render a 3rd party library inside of the Popover.Panel component, and the 3rd party component happens to render in its own Portal component and you interact with that component then we don't want it to close either.

If the components are using our Portal component, then everything is fine, but if they are not using ours, then we have to make some assumptions. Right now, we assume that your applications is rendered in the "main" tree:

<body>
  <div id="root"> <!-- Main tree -->
    <div>
        <Popover>...</Popover>
    </div>
    <div>
      <!-- Sibling -->
    </div>
  </div>

  <!-- Other branches -->
  <known-portal-roots />
  <3rd-party-portal-roots />
  <browser-extensions />
</body>

All of the interactions inside the Main tree that are not happening inside the Popover are considered "outside" of the Popover. But everything happening in the other branches from the root (typically the parts we don't have control over) are considered "allowed" (this is where those notifications or 3rd party libraries are typically rendered).

Next, we require some element in the main tree so that we can run checks against this element compared to the event.target elements for outside click behaviour. This way we can determine where elements are and if they should trigger an outside click or not.

Our Dialog and Popover components render a hidden <MainTreeNode /> component for this. But every Popover component did this, even if you are in a Popover.Group. This resulted in a confusisng DOM structure:

<div> <!-- Popover.Group -->
  <div></div> <!-- Popover -->
  <div></div> <!-- MainTreeNode -->

  <div></div> <!-- Popover -->
  <div></div> <!-- MainTreeNode -->

  <div></div> <!-- Popover -->
  <div></div> <!-- MainTreeNode -->
</div>

If you used ul and li elements, this becomes:

<ul> <!-- Popover.Group -->
  <li></li> <!-- Popover -->
  <div></div> <!-- MainTreeNode -->

  <li></li> <!-- Popover -->
  <div></div> <!-- MainTreeNode -->

  <li></li> <!-- Popover -->
  <div></div> <!-- MainTreeNode -->
</ul>

To solve this, this PR will move the MainTreeNode after the Popover.Group instead, which results in:

<div> <!-- Popover.Group -->
  <div></div> <!-- Popover -->
  <div></div> <!-- Popover -->
  <div></div> <!-- Popover -->
</div>
<div></div> <!-- MainTreeNode -->

If you used ul and li elements, this becomes:

<ul> <!-- Popover.Group -->
  <li></li> <!-- Popover -->
  <li></li> <!-- Popover -->
  <li></li> <!-- Popover -->
</ul>
<div></div> <!-- MainTreeNode -->

@vercel
Copy link

vercel bot commented Aug 2, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 2, 2023 11:22am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 2, 2023 11:22am

@RobinMalfait RobinMalfait merged commit 8a37854 into main Aug 2, 2023
@RobinMalfait RobinMalfait deleted the fix/move-maintree-node-to-popover-group branch August 2, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@RobinMalfait