Create a react portal wrapper

[jaril]

For making the fix for #5944 more robust.

Walkthrough: https://www.loom.com/share/668cdc9557f444fab81727742981df70

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/recordreplay/devtools/8CBLxF3kQhz4TWeHphr2k8k5hDyJ
✅ Preview: https://devtools-git-fork-jaril-0324-react-portal-recordreplay.vercel.app

[jonbell-lot23]

Hello! Thanks for taking a look, and I see what you're doing here, but I disagree with it.

This fix makes an assumption that text-menu-color doesn't need to exist because it's the same as theme-body-color. And with that assumption in hand, this fix focuses more on the cascade.

But our menu background color is pretty light, and it's likely we'll end up making a new text-menu-color that actually is different from body-color. I split out these colors on purpose, because menu elements are their own components, and I need to be able to easily change menu colors without having to untangle them from other colors.

To me, the larger issue isn't the literal color we're using but a higher-level thing: no one should be setting a background color without also setting a foreground color, or else it will break in dark or light theme. That's been where most of our regressions are coming from, and I propose two or three things:

1. We shouldn't use things like bg-white or text-black -- everything should be set with variables and all colors need a foreground and background color.
2. We need to check new UI in both themes as part of our release process
3. We might want to write a quick bit of code that checks that color is always paired with a background color (not sure how feasible this is)

[jaril]

Agreed on todo items #1 and #2. I was initially straight-up opposed to #3 (reasons below), but I want to let it sit for a second.

This fix makes an assumption that text-menu-color doesn't need to exist because it's the same as theme-body-color. And with that assumption in hand, this fix focuses more on the cascade.

👍

But our menu background color is pretty light, and it's likely we'll end up making a new text-menu-color that actually is different from body-color. I split out these colors on purpose, because menu elements are their own components, and I need to be able to easily change menu colors without having to untangle them from other colors.

Earned differences — we should put it in at the point where we're convinced there is a separate color. I'm not a fan of having extra plumbing just in case.

To me, the larger issue isn't the literal color we're using but a higher-level thing: no one should be setting a background color without also setting a foreground color, or else it will break in dark or light theme. That's been where most of our regressions are coming from, and I propose two or three things:

This introduces a hard requirement that, when it's forgotten (and it will), opens us up to bugs that we wouldn't have otherwise.

1. We shouldn't use things like bg-white or text-black -- everything should be set with variables and all colors need a foreground and background color.

Agreed. I've been operating under the assumption that directly reaching for a color that's not declared in a tailwind variable is a big no-no, and when it does happen, to bring it up to you for paint matching.

2. We need to check new UI in both themes as part of our release process

Sure—we've bit the bullet on two modes so we should be doing that anyway.

3. We might want to write a quick bit of code that checks that color is always paired with a background color (not sure how feasible this is)

Still not a big fan of this.

[jaril]

Okay made my peace with it. Put back the manual text color since the main issue I had with this is that even if the text color hadn't been specified it should have tried to go up and find some reasonable color. Using the new AppContainerPortal takes care of that.