chore(site): add custom popover component #10319

BrunoQuaresma · 2023-10-17T17:09:38Z

Usage:

<Popover>
  <PopoverTrigger>
    <button>Open</button>
  </PopoverTrigger>
  <PopoverContent>
    Some content here!
  </PopoverContent>
</Popover>

or

<Popover>
  {(popover) => (
    <>
      <PopoverTrigger>
        <button>Open</button>
      </PopoverTrigger>
      <PopoverContent>
        Some content here!
      </PopoverContent>
    </>
  )}
</Popover>

BrunoQuaresma · 2023-10-17T17:14:50Z

@Parkreiner about using ref inside the render phase, I think it is ok for now since it is how MUI works and we are using it. I would guess they are going to update it if it becomes a problem since they have a huge user base.

…r mode

BrunoQuaresma · 2023-10-18T13:37:37Z

@Parkreiner it is done. The only place I didn't change was the HelpTooltip because I wanted to refactor it later. I think its usage is kinda bad and needs some deep refactoring.

Parkreiner

Looks really good to me overall, and it's good to hear that the swapping process wasn't a nightmare.

Going to approve to avoid blocking you, but still left some comments for the new implementation

Parkreiner · 2023-10-18T17:28:12Z

site/src/components/Popover/Popover.tsx

+
+type TriggerMode = "hover" | "click";
+
+type TriggerRef = React.RefObject<HTMLElement>;


Do you think it's worth genericizing this? Something like:

type TriggerRef <Ele extends HTMLELement = HTMLElement> = React.RefObject<Ele>;

Hm... I don't think so. I think HTMLElement is generic enough. Do you have any use case in mind?

Well, genericizing might be the wrong word, but basically, adding type parameters. We're not exporting TriggerRef, but we are exporting the hook that contains it, and I'm worried that someone might try to reuse the ref from the hook for some other logic, and in some cases, HTMLElement might cause type mismatches because it isn't specific enough

I think it is okay for now, I would not think too much about all possible use cases right now but only on the use cases that we have. We can change it in the future if it is needed.

Parkreiner · 2023-10-18T17:31:13Z

site/src/components/Popover/Popover.tsx

+
+type TriggerElement = ReactElement<{
+  onClick?: () => void;
+  ref: TriggerRef;


Does this work with the intrinsic elements only (div, h1, etc.), or does it also work with any component that works with forwardRef?

I know Radix uses React's ForwardRefExoticComponent type under the hood, but I don't really know what it does, or if it's necessary here

I know it works with any other elements like <Button> that supports forwardRef. ForwardRefExoticComponent looks dark magic 😆

Yeah...it scares me, too

Parkreiner · 2023-10-18T17:32:38Z

site/src/components/Popover/Popover.tsx

+);
+
+export const Popover = (props: {
+  children: ReactNode | ((popover: PopoverContextValue) => ReactNode); // Allows inline usage


Do you feel like this should support arrays, too?

I'm quite sure ReactNode does support it already but if not, I think we can add it when we need it.

site/src/components/Popover/Popover.tsx

Parkreiner · 2023-10-18T17:48:02Z

site/src/components/Popover/Popover.tsx

@@ -0,0 +1,163 @@
+import {


I really like this new implementation! Definitely less chaotic than what I had lol

site/src/components/Popover/Popover.tsx

Parkreiner · 2023-10-18T19:57:22Z

site/src/components/Popover/Popover.tsx

+    },
+  };
+
+  return cloneElement(props.children, {


Actually, I thought of one more suggestion for the trigger: is there a way for you to merge in the aria-haspopup and aria-owns props in as well, just to make sure that the trigger is getting picked up properly by screen readers?

Sure, I will take a look and see how to add them.

Do you have good material about that? I'm not sure how to make this happen 🤔

I actually saw it in MUI's documentation – it's in the source code for the hover popover example:
https://mui.com/material-ui/react-popover/

I'm not sure how the React.clone operation would need to be changed, but two properties would need to be added in:

{ "aria-owns": isOpen ? "some-id-value" : undefined, "aria-haspopup": true }

But I can also take a look into this later today

BrunoQuaresma added 2 commits October 17, 2023 16:33

Rename component to Popover

dc9d55d

Refactor PopoverContainer to be Popover

f549f29

BrunoQuaresma requested a review from Parkreiner October 17, 2023 17:09

BrunoQuaresma self-assigned this Oct 17, 2023

BrunoQuaresma added 14 commits October 17, 2023 17:26

Add horizontal prop

ec0c129

Replace popover on UserDropdown

6daae49

Replace workspace stats popover

412c066

Remove not wanted build info

f107d6d

Refactor popover on icon field

b607ddf

Replace on port forward button

f084d68

Replace popover on ssh button

dcea0bc

Replace popover on date range

fa3d180

Replace popover on build parameters popover

1784025

Replacce popover on edit roles button

09e5f55

Add hover mode

1f2bc8b

Replace popover on terminal

6e0a5d9

Add better comment explaining the reasons of removing spacing on hove…

4eb6f43

…r mode

Add ESLint roles

ba1681f

Parkreiner approved these changes Oct 18, 2023

View reviewed changes

Parkreiner reviewed Oct 18, 2023

View reviewed changes

BrunoQuaresma added 4 commits October 19, 2023 11:15

Use onPointer events instead of mouse

591bea5

change open to isOpen

9a5b238

Fix storybook

5dc128c

Add back the dropdown arrow on user menu

d704c59

BrunoQuaresma merged commit f677c44 into main Oct 19, 2023

BrunoQuaresma deleted the bq/custom-popover-component branch October 19, 2023 12:13

github-actions bot locked and limited conversation to collaborators Oct 19, 2023


		type TriggerMode = "hover" \| "click";

		type TriggerRef = React.RefObject<HTMLElement>;

chore(site): add custom popover component #10319

chore(site): add custom popover component #10319

Uh oh!

Conversation

BrunoQuaresma commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrunoQuaresma commented Oct 17, 2023

Uh oh!

BrunoQuaresma commented Oct 18, 2023

Uh oh!

Parkreiner left a comment

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!

Parkreiner Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Parkreiner Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrunoQuaresma commented Oct 17, 2023 •

edited

Loading

Parkreiner Oct 18, 2023 •

edited

Loading

Parkreiner Oct 18, 2023 •

edited

Loading