feat(site): add WorkspacesButton component #10011

Parkreiner · 2023-10-03T12:58:33Z

Solves #9335 and lays some groundwork for solving #10012 and #10014

Really sorry this took so long. This PR updates the button for creating workspaces with a more conspicuous dropdown menu.

Things changed

Replaces Workspaces link with dedicated button that displays a full dropdown, complete with proper hover/focus styling
Creates two new top-level components:
- PopoverContainer simplifies the process for using MUI's kind of wonky Popover component, and removes the need to set up custom hooks and refs before even being able to start using the component
- OverflowY provides a reusable "stretchy" component for wrapping any content and making sure that it scrolls properly, but also shrinks correctly as the viewport's height decreases.
Implements the Workspaces button by composing over these two new components

Screen.Recording.2023-10-03.at.9.06.01.AM.mov

Left to do (this PR)

See what Storybook stories need to be added for the top-level components

Left to do (separate PRs during downtime)

I created custom hooks for handling image pre-loading (feat: add image pre-loading hooks #10015), to maximize the chances that the popover doesn't ever render with empty images. Both hooks seem to be fully working, but there are zero tests for them yet. Didn't feel comfortable pushing this out without making sure we have assurances first.
I also need to update the other dropdown components to use PopoverContainer and OverflowY
Tried migrating this to use Emotion, but I ran into some cases where the spacing broke. Aiming to public the code as-is, and then circle back for the migration

Out of scope

This does not address some concerns the team has had with the primary call-to-action button blending into the background because it's so muted. I will be opening a separate issue about that later on tonight.

Other notes

I made another component for trying to solve all our filtering issues, trying to solve every possible filtering need the codebase could have.
- I'm sure there are some lessons to take from it, but my gut feeling for this kind of thing is "it's really good at making you feel smart, really bad at saddling you with abstractions that don't line up with what you're trying to do in the moment"

site/src/pages/WorkspacesPage/WorkspacesButton.tsx

Kira-Pilot · 2023-10-04T17:20:23Z

site/src/components/PopoverContainer/PopoverContainer.tsx

+
+  // Ref value is for effects and event listeners; state value is for React
+  // renders. Have to duplicate state because after the initial render, it's
+  // never safe to reference ref contents inside a render path, especially with


Because React never rerenders the component? Why is particularly the case with React 18?

So technically it's a concern with all versions of React that use hooks – the React team really wants you to treat refs like black boxes during renders. As in, the ref itself is fine – it's just a box for storing values, and you can work with the box and attach it to elements. But the only times it's safe to peak into the box to read from it or update the contents are one of two situations:

On the very first render when you're setting up the ref for the first time. For example:

// This is only safe if the other effects and event handlers can't change the ref to null const abortControllerRef = useRef<AbortController>(null!); if (abortController.current === null) { abortControllerRef.current = new AbortController(); }

Inside the various useEffect hooks or event handlers

But it's more of a problem with React 18's concurrency mode, which has the ability to stop/throw away, de-prioritize, and redo renders. They can only do that because renders are supposed to be pure. Reading from/writing refs is technically a side effect/"side cause", though, so there's a risk that as their render engine changes (which might happen soon with React Forget), there's more risks of things breaking if the rules aren't followed 100%

They can only do that because renders are supposed to be pure, and reading from/writing refs is technically a side effect/"side cause", so there's a risk that as their render behavior changes (which might happen soon with React Forget), there's more risks of things breaking if the rules aren't followed 100%

Ahhh, makes sense. Thanks for the explanation!

Kira-Pilot · 2023-10-04T17:22:43Z

site/src/components/PopoverContainer/PopoverContainer.tsx

@@ -0,0 +1,149 @@
+/**
+ * @file Abstracts over MUI's Popover component to simplify using it (and hide)
+ * some of the wonkier parts of the API.


Might be useful to hint at some of the wonkier API parts of MUI, or maybe link to their docs.

Yeah, good point. I guess if I had to summarize up my concern, it feels to me like their current API (as in, the entire conceit of it) defeats the point of React?

Like, the point of JSX is that you don't have to touch live DOM nodes during actual render logic, and only have to worry about them when you use effects and event handlers as escape hatches. But MUI has you take a whole real DOM node (which is physically incapable of existing on the first render), and store it in React state, to be accessed in renders

It feels backwards to me, and I've seen the component get set up multiple different ways in the codebase to make it happy, when it feels like there should be one obvious way of doing things

it feels to me like their current API (as in, the entire conceit of it) defeats the point of React?

lolol

Your abstraction makes sense to me. Maybe you could make a contribution!

Kira-Pilot · 2023-10-04T17:31:06Z

site/src/pages/WorkspacesPage/WorkspacesButton.tsx

+      sx={{ display: "flex", flexFlow: "column nowrap" }}
+      anchorButton={
+        <Button startIcon={<AddIcon />} variant="contained">
+          Create Workspace&hellip;


Why the ellipsis?

Just a UI concern – it just helps communicate that there's no immediate action when you click the button, and you'll see more options when you do.

Kayla had a thread about this recently:
https://codercom.slack.com/archives/CJURPL8DN/p1694725326331259

Kira-Pilot · 2023-10-04T17:47:04Z

This looks great and really well executed. I'm excited about your new, shared components, especially the accessible popover.

Two minor things I noticed:

Seems like there's some extra spacing on small screen widths:

2. I noticed the dropdown component isn't cleaned up when navigating to the Create Workspace page:

Parkreiner · 2023-10-04T18:53:51Z

This looks great and really well executed. I'm excited about your new, shared components, especially the accessible popover.

Two minor things I noticed:

Seems like there's some extra spacing on small screen widths:

2. I noticed the dropdown component isn't cleaned up when navigating to the Create Workspace page:

Oh, good catch! Will get those fixed up

Parkreiner · 2023-10-04T20:23:04Z

@Kira-Pilot Could I get more info on what you did to get the popover to stick around longer than it should've, and where/how you ran the code?

Did you basically:

Click the button to open the dropdown
Click an option in the menu
The page navigated, but the dropdown just stuck around? Did the dropdown eventually go away (but was just slow), or did it just never disappear?

The reason I'm asking is because right now the component assumes that MUI will just unmount the entire dropdown if the button has no reason to be on screen anymore, which seems like it should be enough. But I've been having trouble replicating the issue.

Even this code, which is supposed to be the nuclear option that wipes everything away when the component unmounts, never actually triggers:

const popoverRef = useRef<HTMLDivElement>(null);

useEffect(() => {
  const popover = popoverRef.current;
  
  return () => {
    if (popover !== null) {
      popover.innerHTML = "";
      console.log("Nodes burned to the ground");
    }
  };
}, []);

return <Popover ref={popoverRef} {...otherProps}>{children}</Popover>

Parkreiner · 2023-10-05T13:24:55Z

Okay, took a lot of research, and I had to do some slightly cursed things under the hood, but there's a solution in place for making sure the popover closes before a Router page transition. The API should hopefully be really straightforward, too.

Working on styling fixes now

Kira-Pilot · 2023-10-05T14:57:50Z

@Parkreiner sounds like you got it figured out but just to reply to your comment: yes, those are the steps to reproduce. I would see this issue only after a page refresh. The popover would eventually disappear after lingering for a bit. Glad you were able to diagnose!

Kira-Pilot · 2023-10-05T15:00:25Z

Looks like there are a couple of storybook errors - maybe we're missing a provider somewhere?

Kira-Pilot · 2023-10-05T15:01:36Z

site/src/components/PopoverContainer/PopoverContainer.tsx

+    void Promise.resolve().then(() => {
+      navigate(to, linkProps);
+    });
+  };


Parkreiner · 2023-10-05T15:08:57Z

@Kira-Pilot Yeah, looks like that's on me. I have the button component calling the permissions and organization ID hooks directly, which works for the app, but makes the tests break

Gonna ask about this in the dev channel – I'm guessing our approach is to always parameterize these kinds of things from the pages component

Kira-Pilot · 2023-10-05T15:13:09Z

Gonna ask about this in the dev channel – I'm guessing our approach is to always parameterize these kinds of things from the pages component

Yeah, it's a major bummer and leads to some weird patterns. I am OOO today but I went ahead and approved your PR so you're not blocked; feel free to approve the snapshots yourself when they resolve.

Parkreiner · 2023-10-05T15:13:56Z

Gonna ask about this in the dev channel – I'm guessing our approach is to always parameterize these kinds of things from the pages component

Yeah, it's a major bummer and leads to some weird patterns. I am OOO today but I went ahead and approved your PR so you're not blocked; feel free to approve the snapshots yourself when they resolve.

Okay, sounds good. Thank you!

Parkreiner added 7 commits October 3, 2023 12:20

chore: Add OverflowY component

f0a2aae

chore: Add PopoverContainer component

2420167

chore: Add SearchBox

88d73af

feat: add WorkspacesButton

209eed4

chore: Install MUI utils package

9300b11

chore: integrate WorkspacesButton

2b13f1e

chore: reorganize files

e86207e

github-actions bot assigned Parkreiner Oct 3, 2023

Parkreiner changed the title ~~feat (site): add WorkspacesButton component~~ feat(site): add WorkspacesButton component Oct 3, 2023

fix: resolve hover state visual glitch

5b249d5

Parkreiner commented Oct 3, 2023

View reviewed changes

site/src/pages/WorkspacesPage/WorkspacesButton.tsx Outdated Show resolved Hide resolved

Parkreiner added 4 commits October 3, 2023 19:27

chore: Add story for OverflowY

b1e1271

fix: remove dynamic name from OverflowY story

b367495

chore: update stories again

5a34769

fix: remove all references to icons (for now)

b9f6cb8

Parkreiner marked this pull request as ready for review October 3, 2023 21:53

Parkreiner requested review from a team, code-asher and Kira-Pilot and removed request for a team and code-asher October 3, 2023 21:53

Kira-Pilot reviewed Oct 4, 2023

View reviewed changes

refactor: move flex shrink to be OverflowY concern

1996e2b

Parkreiner added 2 commits October 4, 2023 21:51

fix: remove needless render key

5ecbbe6

fix: make sure popover closes before navigation

55ab10a

Parkreiner added 6 commits October 5, 2023 13:41

refactor: clean up WorkspacesButton to use more native MUI

1a01127

Merge branch 'main' into mes/workspace-button-2

80928c6

fix: update integration into rest of view

55c6061

fix: remove JS security concern

5aea06f

refactor: parameterize button language

2aee4fe

revert: undo sql/go file change

12ac56f

Kira-Pilot reviewed Oct 5, 2023

View reviewed changes

site/src/components/PopoverContainer/PopoverContainer.tsx

void Promise.resolve().then(() => {

navigate(to, linkProps);

});

};

Copy link

Member

Kira-Pilot Oct 5, 2023

Choose a reason for hiding this comment

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

⭐

Kira-Pilot approved these changes Oct 5, 2023

View reviewed changes

Parkreiner added 5 commits October 5, 2023 18:37

fix: remove permissions dependency

99f2656

fix: simplify button prop types

430e30c

fix: lift data dependencies to page component

ff37ab5

refactor: clean up props

1b934ab

fix: update dependencies again for Storybook

84c6642

Parkreiner enabled auto-merge (squash) October 5, 2023 20:45

Parkreiner merged commit 2d6c4fe into main Oct 5, 2023

Parkreiner deleted the mes/workspace-button-2 branch October 5, 2023 20:46

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(site): add WorkspacesButton component #10011

feat(site): add WorkspacesButton component #10011

Parkreiner commented Oct 3, 2023 •

edited

Loading

Kira-Pilot Oct 4, 2023

Parkreiner Oct 4, 2023

Parkreiner Oct 4, 2023 •

edited

Loading

Kira-Pilot Oct 4, 2023

Kira-Pilot Oct 4, 2023

Parkreiner Oct 4, 2023 •

edited

Loading

Kira-Pilot Oct 4, 2023 •

edited

Loading

Kira-Pilot Oct 4, 2023

Parkreiner Oct 4, 2023 •

edited

Loading

Kira-Pilot commented Oct 4, 2023

Parkreiner commented Oct 4, 2023

Parkreiner commented Oct 4, 2023 •

edited

Loading

Parkreiner commented Oct 5, 2023 •

edited

Loading

Kira-Pilot commented Oct 5, 2023

Kira-Pilot commented Oct 5, 2023

Kira-Pilot Oct 5, 2023

Parkreiner commented Oct 5, 2023

Kira-Pilot commented Oct 5, 2023

Parkreiner commented Oct 5, 2023

feat(site): add WorkspacesButton component #10011

feat(site): add WorkspacesButton component #10011

Conversation

Parkreiner commented Oct 3, 2023 • edited Loading

Things changed

Left to do (this PR)

Left to do (separate PRs during downtime)

Out of scope

Other notes

Kira-Pilot Oct 4, 2023

Choose a reason for hiding this comment

Parkreiner Oct 4, 2023

Choose a reason for hiding this comment

Parkreiner Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Kira-Pilot Oct 4, 2023

Choose a reason for hiding this comment

Kira-Pilot Oct 4, 2023

Choose a reason for hiding this comment

Parkreiner Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Kira-Pilot Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Kira-Pilot Oct 4, 2023

Choose a reason for hiding this comment

Parkreiner Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Kira-Pilot commented Oct 4, 2023

Parkreiner commented Oct 4, 2023

Parkreiner commented Oct 4, 2023 • edited Loading

Parkreiner commented Oct 5, 2023 • edited Loading

Kira-Pilot commented Oct 5, 2023

Kira-Pilot commented Oct 5, 2023

Kira-Pilot Oct 5, 2023

Choose a reason for hiding this comment

Parkreiner commented Oct 5, 2023

Kira-Pilot commented Oct 5, 2023

Parkreiner commented Oct 5, 2023

Parkreiner commented Oct 3, 2023 •

edited

Loading

Parkreiner Oct 4, 2023 •

edited

Loading

Parkreiner Oct 4, 2023 •

edited

Loading

Kira-Pilot Oct 4, 2023 •

edited

Loading

Parkreiner Oct 4, 2023 •

edited

Loading

Parkreiner commented Oct 4, 2023 •

edited

Loading

Parkreiner commented Oct 5, 2023 •

edited

Loading