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.

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

Popover component for floating content next to a target element #16481

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

izmy
Copy link
Contributor

@izmy izmy commented Jan 20, 2025

Description

Previously, the dropdown could render outside the viewport, particularly on smaller screens. So I created a new Popover component to keep content within the viewport. The component monitors whether the content is visible and dynamically adjusts its position when necessary. I used the @floating-ui/react library for this functionality, which is already used for tooltips.

I also refactored the Dropdown component, then separated the Menu file, and created an example in the storybook.

Related Issue

Resolve #12939

Screenshots:

image

image image

@izmy izmy requested review from tomasklim and TomasBoda January 20, 2025 16:50
@izmy izmy self-assigned this Jan 20, 2025
@izmy izmy force-pushed the feat/popover-component branch 4 times, most recently from 24ada5f to 8c168a2 Compare January 21, 2025 10:51
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful 🚀 🥳

packages/theme/src/zIndices.ts Show resolved Hide resolved
Copy link
Contributor

@jvaclavik jvaclavik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this PR very much. Good job @izmy 👏

I added few comments, but mostly nitpicks, no real blocker from my side.

packages/components/src/components/Popover/Popover.tsx Outdated Show resolved Hide resolved
packages/components/src/components/Popover/Popover.tsx Outdated Show resolved Hide resolved
packages/components/src/components/Popover/Popover.tsx Outdated Show resolved Hide resolved
);
});

export function Popover({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offtopic: shouldn't we also abstract Tooltip to Popover similarly like Dropdown? Does it make sense? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Tootip has a similar implementation, because it also uses @floating-ui/react, so I think it makes sense for me.

@izmy izmy force-pushed the feat/popover-component branch from 5c275fd to 4db6013 Compare January 27, 2025 11:49
@izmy izmy requested review from jvaclavik and tomasklim January 27, 2025 12:08
@izmy izmy force-pushed the feat/popover-component branch from 4db6013 to 80fc82f Compare January 27, 2025 12:08
@izmy izmy requested a review from a team as a code owner January 28, 2025 12:24
@izmy izmy force-pushed the feat/popover-component branch from d9e8512 to ee8217b Compare January 28, 2025 12:38
@tomasklim
Copy link
Member

/rebase

Copy link

Copy link

Rebasing failed, please rebase manually.

@izmy izmy force-pushed the feat/popover-component branch from ee8217b to 641394f Compare January 28, 2025 13:44
@tomasklim tomasklim merged commit bef1b3c into develop Jan 28, 2025
32 checks passed
@tomasklim tomasklim deleted the feat/popover-component branch January 28, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown can render outside of viewport
3 participants