Skip to content

[Popover] Component and Hook#381

Merged
atomiks merged 29 commits intomui:masterfrom
atomiks:feat/Popover
Jun 25, 2024
Merged

[Popover] Component and Hook#381
atomiks merged 29 commits intomui:masterfrom
atomiks:feat/Popover

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 type: new feature Expand the scope of the product to solve a new problem. component: popover Changes related to the popover 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
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: popover Changes related to the popover component. type: new feature Expand the scope of the product to solve a new problem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[popover] Implement Popover

5 participants