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 and Hook #381

Merged
merged 29 commits into from
Jun 25, 2024
Merged

[Popover] Component and Hook #381

merged 29 commits into from
Jun 25, 2024

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented May 1, 2024

@atomiks atomiks marked this pull request as draft May 1, 2024 01:07
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels May 2, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 22, 2024
@danilo-leal danilo-leal added new feature New feature or request component: popover The React component. labels May 22, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 29, 2024
@mui-bot
Copy link

mui-bot commented May 29, 2024

Netlify deploy preview

https://deploy-preview-381--base-ui.netlify.app/

Generated by 🚫 dangerJS against 9ed3563

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 29, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 29, 2024
@atomiks atomiks force-pushed the feat/Popover branch 2 times, most recently from 3b4e961 to 6606738 Compare May 31, 2024 11:17
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 3, 2024
@colmtuite
Copy link
Contributor

@atomiks

  • It doesn’t trap focus. Why not?
  • I think the delayType prop should default to hover because I think that is the expected behaviour for most people.
  • I think the default delay for openOnHover is too short, especially if we change the delayType prop default to hover. 500ms would be much better imo.

@atomiks
Copy link
Contributor Author

atomiks commented Jun 10, 2024

It doesn’t trap focus. Why not?

Popovers are non-modal elements that don't trap focus by default. They don't have a backdrop and don't make things inert behind them. They do manoeuvre focus to the correct tabbable elements in the DOM tree however when tabbing or moving through elements with the virtual cursor.

I think the delayType prop should default to hover because I think that is the expected behaviour for most people.

This one is pretty subjective and I think we should match whatever Tooltip does - as mentioned earlier, the rest type has intentionality benefits especially when the delay is very short to prevent it from opening when the cursor is gliding across the UI with no intention to interact with the popover's trigger.

I think the default delay for openOnHover is too short, especially if we change the delayType prop default to hover. 500ms would be much better imo.

Unlike tooltips whose trigger performs an action unrelated to the tooltip, the delay seems like it should be be much shorter to show the popover quickly on hover, as its trigger's only purpose is to open the popover itself?

@atomiks atomiks marked this pull request as ready for review June 12, 2024 00:35
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 12, 2024
docs/pages/experiments/popover.tsx Outdated Show resolved Hide resolved
packages/mui-base/test/conformanceTests/utils.ts Outdated Show resolved Hide resolved
packages/mui-base/src/utils/BaseUI.types.ts Outdated Show resolved Hide resolved
packages/mui-base/src/Popover/index.ts Outdated Show resolved Hide resolved
packages/mui-base/src/Popover/Root/usePopoverRoot.ts Outdated Show resolved Hide resolved
packages/mui-base/src/Popover/Provider/PopoverProvider.tsx Outdated Show resolved Hide resolved
@colmtuite
Copy link
Contributor

@atomiks Decisions made after a chat:

  • Default both Tooltip and Popover to 500ms delay with delayType=“rest”
  • Keep Backdrop
  • Add Description
  • Keep Close
  • Remove cursor following from Popover. Keep it for Tooltip

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jun 17, 2024
@atomiks atomiks force-pushed the feat/Popover branch 2 times, most recently from 3ae39a8 to 46026c0 Compare June 18, 2024 06:09
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 18, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 25, 2024
@atomiks atomiks merged commit 3226736 into mui:master Jun 25, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: popover The React component. new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Popover] Implement Popover
5 participants