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

New popover #1506

Merged
merged 143 commits into from
Sep 11, 2023
Merged

New popover #1506

merged 143 commits into from
Sep 11, 2023

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Aug 17, 2023

Changes

Continued from #1392

Replaced old tippy-based private Popover component with new floatingui-based private usePopover hook and public Popover component.

Popover defaults to role=dialog and does some focus management, whereas usePopover gives more control and is useful for internal components that bring their own logic for roles and focus management and custom triggers. The public Popover is also used in some internal components that do not need such custom logic.

Removed old popover.scss; it was mainly used for reverting tippy styles (which don't exist anymore). The new popover does not need any CSS but also does not come with any animations (can be added in the future).

The public Popover has an applyBackground prop which converts it into a Surface. Since this is a new component, I've decided not expose all floating-ui props to keep API surface small; we can re-evaluate in the future based on user requests.

Testing

Thoroughly tested all affected components in storybook and vite playground.

Here's a screen recording showing various components in action:

Screen.Recording.2023-09-08.at.12.15.36.PM.mov

Updated all affected unit tests.

Docs

Changesets added. Pending documentation for Popover (future PR).

@mayank99 mayank99 added this to the React 3.0 milestone Aug 17, 2023
@mayank99 mayank99 self-assigned this Aug 17, 2023
@mayank99 mayank99 linked an issue Aug 17, 2023 that may be closed by this pull request
@mayank99 mayank99 marked this pull request as ready for review September 8, 2023 16:11
@mayank99 mayank99 requested review from a team as code owners September 8, 2023 16:11
@mayank99 mayank99 removed request for a team September 8, 2023 16:17
Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Looks good!

// max-height must be on the outermost element for virtual scroll
maxBlockSize:
'calc((var(--iui-component-height) - 1px) * 8.5)',
overflowY: isOverflowOverlaySupported() ? 'overlay' : 'auto',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we are now using iui-menu as the outer element, we don't need to set these inline styles for virtualization

while writing this, i also realized that we can remove iui-scroll and make it the default behavior. this will fix #1301 as well. i will create a separate PR for it

Co-authored-by: Rohan Kadkol <45748283+rohan-kadkol@users.noreply.github.com>
Co-authored-by: Rohan Kadkol <45748283+rohan-kadkol@users.noreply.github.com>
mayank99 and others added 2 commits September 11, 2023 11:39
Co-authored-by: Rohan Kadkol <45748283+rohan-kadkol@users.noreply.github.com>
@r100-stack
Copy link
Member

@gretanausedaite @rohan-kadkol I think the code is ready for review at this point. I'm still working on tests, changesets, PR description, etc.

Ohh, sorry, I missed this message. I reviewed and approved it. There were a few parts that involved concepts that I don't understand too well. But for the parts that I did understand, I reviewed them better.

@mayank99 mayank99 merged commit 39503cf into dev Sep 11, 2023
15 of 16 checks passed
@mayank99 mayank99 deleted the mayank/new-popover branch September 11, 2023 17:30
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
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.

v3: Migrate from tippyjs to floating-ui
3 participants