[PreviewCard] Create new component by atomiks · Pull Request #469 · mui/base-ui

atomiks · 2024-06-27T11:23:16Z

Closes #451

mui-bot · 2024-06-27T11:28:34Z

Netlify deploy preview

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

Generated by 🚫 dangerJS against 54c6c28

colmtuite · 2024-06-27T12:35:26Z

What's the reason behind not opening on focus?

atomiks · 2024-06-27T12:57:50Z

What's the reason behind not opening on focus?

Hovercards (as the name seems to imply) are cards opened only via hover — the interactive content inside is not accessible via keyboard as you might expect if it opened on focus. Ariakit and Mantine don't open theirs on focus, while Radix does however.

colmtuite · 2024-06-27T15:15:07Z

Hovercards (as the name seems to imply) are cards opened only via hover

The name is irrelevant, we could change that to whatever. It's the experience that matters. It's not a well-defined pattern, so we can choose to do whatever we think is best.

the interactive content inside is not accessible via keyboard as you might expect if it opened on focus

It's a sighted only UI though right? And opening on focus still benefits sighted users. Non-sighted users don't benefit from the HoverCard at all, whether we open on focus or not?

atomiks · 2024-06-28T06:10:36Z

It's a sighted only UI though right? And opening on focus still benefits sighted users. Non-sighted users don't benefit from the HoverCard at all, whether we open on focus or not?

The key problem is interactive content inside like a Follow button, there's no way to access that content without focus management if it opens on focus. That's why enforcing it to be hover-only is more appropriate.

Open Hover Card on focus
The Hover Card would steal focus if it moved to the Follow button like other components do with initial focus. The only valid method would be keeping focus on the trigger link, and letting the next tab access the Hover Card's contents. Pressing Esc places focus back onto the trigger, which should re-open the Hover Card since it opens when the trigger gains focus, leading to a loop (unless guarded). This pattern can be confusing - hence "enforcing it to be hover-only is more appropriate."

It looks like Wikipedia's Hover Cards do open on focus, but you can't access the interactive settings icon inside of it. Maybe it's ok that interactive content is inaccessible to keyboard users, for the benefit of being able to see a preview at all?

colmtuite · 2024-07-02T07:49:18Z

@atomiks

The default delay needs to to updated to 600 to match the new Popover/Tooltip timings.
The delay should be present when the popup is triggered by focus.

atomiks · 2024-07-02T07:57:36Z

@colmtuite Why do tooltips have 0 delay but preview cards have the hover's delay when either is triggered on focus? Since both are "secondary", shouldn't they behave the same?

colmtuite · 2024-07-02T08:02:04Z

@atomiks Becase PreviewCard almost always goes on a text link, so the popup is not necessarily desired when you focus it. Whereas Tooltip almost always goes on an icon button, and you might need to know what the button does asap.

Also, PreviewCard is almost always very significantly larger than Tooltip, so it is extremely obnoxious when it instantly appears upon focusing its trigger. The Tooltip is much less obnoxious.

atomiks · 2024-07-02T08:12:52Z

Becase PreviewCard almost always goes on a text link, so the popup is not necessarily desired when you focus it. Whereas Tooltip almost always goes on an icon button, and you might need to know what the button does asap.

Couldn't the same argument be applied to a link, if the text can be as ambiguous as an icon button? In some circumstances you may want to preview of the link's contents as soon as possible as well.

Instagram's preview cards have 0 delay, but Wikipedia's have the delay. I think I do prefer Wikipedia's method (that you're suggesting) because of your next point:

Also, PreviewCard is almost always very significantly larger than Tooltip, so it is extremely obnoxious when it instantly appears upon focusing its trigger. The Tooltip is much less obnoxious.

The cards on Instagram can block a lot of content so this makes more sense to me. An API change to configure hover + focus delay triggers individually may be a bit problematic. I think we can leave the configuration for this for now, and have the delay be applied equally to hover + focus triggers.

colmtuite · 2024-07-02T08:16:19Z

I think we can leave the configuration for this for now, and have the delay be applied equally to hover + focus triggers.

I think it's possible that we don't ever need to configure the triggers independently. But this is a good first step in any case. 👍

mnajdova · 2024-07-09T07:05:37Z

+  const { delayType = 'rest', delay, closeDelay, animated = true } = props;
+
+  const delayWithDefault = delay ?? OPEN_DELAY;
+  const closeDelayWithDefault = closeDelay ?? CLOSE_DELAY;


Suggested change

const { delayType = 'rest', delay, closeDelay, animated = true } = props;

const delayWithDefault = delay ?? OPEN_DELAY;

const closeDelayWithDefault = closeDelay ?? CLOSE_DELAY;

const { delayType = 'rest', delay = OPEN_DELAY, closeDelay = CLOSE_DELAY, animated = true } = props;

Why not use the destructuring for assigning the defaults?

The prop-types/docs api gen doesn't allow variables as defaults when destructuring

Damn, I am adding it on the code-infra topics for next week.

mnajdova · 2024-07-09T07:07:44Z

+   * The side of the anchor element that the preview card element should align to.
+   * @default 'bottom'
+   */
+  side?: Side;


I think we should use rtl safe values here, like start/end. I know the type comes from Floating UI, but we can change the values on a component level.

For example, for alignment, we use: start, center, and end. We would be consistent.

You're right that we can support logical sides on a component level, but they are more complex to understand compared to physical sides, and logical alignments are far more useful for many cases, not only RTL. This is an issue for the anchor positioning hook though, so it shouldn't relate to this PR itself.

michaldudak

Looks good overall. I have a few minor remarks.

michaldudak · 2024-07-19T07:44:04Z

+    className,
+    ownerState,
+    extraProps: otherProps,
+    customStyleHookMapping: {


This could be extracted to a const outside of the component so it's not recreated on every render

I noticed our other components have it inlined sometimes, and useComponentRenderer passes it as a memo dependency, so they'll need to be updated as well

michaldudak

LGTM

atomiks requested review from colmtuite and michaldudak as code owners June 27, 2024 11:23

atomiks marked this pull request as draft June 27, 2024 11:23

atomiks added the component: preview card Changes related to the preview card component. label Jun 27, 2024

atomiks changed the title ~~[HoverCard] Create new component~~ [PreviewCard] Create new component Jun 28, 2024

atomiks added 10 commits July 2, 2024 18:16

[HoverCard] Create new component

570623b

Add initial tests

555a9af

Add Hover Card to docs data-display

ca5a522

Adjust delays

47de95e

Adjust delays

253400d

Rename to PreviewCard

774c990

Fix backdrop filename

9db606e

Rename to Preview Card

fec414b

Remove Hover Card

d1dbc11

Update APIs

1b9adbd

atomiks force-pushed the feat/HoverCard branch from 9b848fb to 1b9adbd Compare July 2, 2024 09:09

atomiks added 2 commits July 2, 2024 19:29

Update

ba203f5

Remove unused file

d888320

atomiks marked this pull request as ready for review July 2, 2024 09:53

mnajdova reviewed Jul 9, 2024

View reviewed changes

michaldudak reviewed Jul 19, 2024

View reviewed changes

atomiks added 4 commits July 22, 2024 13:49

Feedback

4c4aab7

Reuse internal types

b133abc

Merge branch 'master' into feat/HoverCard

185611d

Docs

3ef5648

michaldudak approved these changes Jul 22, 2024

View reviewed changes

colmtuite approved these changes Jul 22, 2024

View reviewed changes

Remove skip prop

54c6c28

atomiks merged commit 00aebe3 into mui:master Jul 23, 2024

atomiks deleted the feat/HoverCard branch July 23, 2024 01:22

Uh oh!

Uh oh!

Conversation

atomiks commented Jun 27, 2024 • edited by michaldudak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Uh oh!

colmtuite commented Jun 27, 2024

Uh oh!

atomiks commented Jun 27, 2024

Uh oh!

colmtuite commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atomiks commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colmtuite commented Jul 2, 2024

Uh oh!

atomiks commented Jul 2, 2024

Uh oh!

colmtuite commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atomiks commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colmtuite commented Jul 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaldudak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaldudak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

atomiks commented Jun 27, 2024 •

edited by michaldudak

Loading

mui-bot commented Jun 27, 2024 •

edited

Loading

colmtuite commented Jun 27, 2024 •

edited

Loading

atomiks commented Jun 28, 2024 •

edited

Loading

colmtuite commented Jul 2, 2024 •

edited

Loading

atomiks commented Jul 2, 2024 •

edited

Loading