Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

chore(site): convert more components from Emotion to TailwindCSS#19719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Parkreiner merged 30 commits intomainfrommes/emo-01c
Sep 17, 2025

Conversation

Parkreiner
Copy link
Member

@ParkreinerParkreiner commentedSep 6, 2025•
edited
Loading

Changes made

  • Patched ReactCSSProperties type to add support for custom CSS properties
  • Updated several of the components in thecomponents directory to Tailwind
  • Updated most of theWorkspacePageBuildView component to Tailwind to account for CSS specificity changes
  • UpdatedSearch to address accessibility violation and removed all MUI logic
  • UpdatedSearch stories (added new story, decoupled all stories from single decorator)
  • UpdatedautoFocus behavior inSearchField
  • Updated the styling forWorkspacePageBuildView to make sure the tabs had enough padding
  • Fixed layout effect inWorkspacePageBuildView to fire correctly

@ParkreinerParkreiner self-assigned thisSep 6, 2025
@ParkreinerParkreiner changed the titlechore(site): convert more component from Emotion to TailwindCSSchore(site): convert more components from Emotion to TailwindCSSSep 8, 2025
key={option.value}
className={cn(
"data-[disabled]:bg-content-disabled data-[disabled]:text-surface-tertiarydata-[disabled]:hover:bg-content-disabled",
"data-[disabled]:bg-content-disabled data-[disabled]:text-surface-tertiary data-[disabled]:hover:bg-content-disabled",
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

We had a missing space before, so two of the classes would just never trigger

Copy link
Member

Choose a reason for hiding this comment

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

I love Tailwind

Comment on lines +1 to +7
declare module"react"{
interfaceCSSProperties{
[key: `--${string}`]:string|number|undefined;
}
}

export{};
Copy link
MemberAuthor

@ParkreinerParkreinerSep 8, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Felt like this would be useful. It's always been valid React, but it's just never been baked into the type definitions

Used for setting a custom CSS variable that's scoped to a specific element

Copy link
Member

Choose a reason for hiding this comment

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

omg thank you

Comment on lines -8 to -12
(Story)=>(
<Search>
<Story/>
</Search>
),
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This was making it so that every time you tried to mountSearchInput, you would always getSearch wrapped around it, which made theSearchEmpty story a pain to write

</label>
<input
ref={$$ref}
id={id}
Copy link
MemberAuthor

@ParkreinerParkreinerSep 8, 2025•
edited
Loading

Choose a reason for hiding this comment

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

We were adding an ID to a label, but never associated it with the input

type="text"
placeholder="Search..."
css={SearchInputStyles.input}
className="text-inherit h-full border-0 bg-transparent grow basis-0 outline-none pl-4 placeholder:text-content-secondary"
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I don't know if this CSS is 100% best practices, but we have a bunch offlex: 1 in the codebase, which is equivalent todisplay: flex; flex-grow: 1; flex-basis: 0

I think that most of the time, we just wanteddisplay: flex in the CSS, but I didn't have time to go through the ramifications of changing all the styles

/**
* Reusable styles for consumers of the base components
*/
exportconstsearchStyles={
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This was only ever used forWorkspaceButton, so I just moved the styles there

Copy link
Member

Choose a reason for hiding this comment

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

very nice. this should've been avariant prop or something if we actually cared about reuse. I like going with the simplest solution tho.

inputRef.current?.focus();
node?.focus();
}
});
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This wasn't hard to fix, but it was a nasty bug, and I think the only reason we haven't had a problem is because we never had any components withautoFocus set totrue

margin:"0px",
flexWrap:"wrap",

[theme.breakpoints.down("md")]:{
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

We had a bunch of logic like this, where we were basically doing responsive design backwards

I tried to invert the style declarations so that the mobile layouts were the bases, and the desktop styles became the overrides. There are some changes to the logic, just because MUI and Tailwind have differentmd breakpoints, but it didn't seem like a big enough deal to add more tokens to the Tailwind config

{...linkProps}
className={cn(
`text-sm text-content-secondary no-underline font-medium py-3 px-1mr-6hover:text-content-primary rounded-md
`text-sm text-content-secondary no-underline font-medium py-3 px-1 hover:text-content-primary rounded-md
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

For some reason, we were hard-coding a margin value to every single tab item, instead of letting the parent handle the layout concerns

<Tabsactive={tabState.value}>
<TabsList>
<TabLinkto={`?${LOGS_TAB_KEY}=build`}value="build">
<TabsListclassName="gap-0">
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I didn't want to make too many sweeping changes, but the styling for the tabs was super jacked up. The tab text had zero padding, and was flush with the container's left edge

resizeObserver.disconnect();
};

syncParentSize();
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

We weren't calling any state updater functions in the layout effect's main body, which defeats the entire point of making this a layout effect over a normal effect

Copy link
Member

Choose a reason for hiding this comment

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

I don't really see why we even need to involve the state cycle here. why not justcontentEl.style.height = contentEl.parentElement.clientHeight?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

We could do that. I guess the main concern is that if we're going to be changing height without making React aware of it, there's a risk that React could wipe the height away on re-renders

@Parkreiner
Copy link
MemberAuthor

Turning this back into a draft at the moment. Turns out that a bunch of our CSS styles just happened to work by luck, and trying update/tease things apart broke a bunch of implicit styles

@ParkreinerParkreiner removed the request for review fromaslilacSeptember 8, 2025 15:57
@ParkreinerParkreiner marked this pull request as ready for reviewSeptember 8, 2025 16:34
@Parkreiner
Copy link
MemberAuthor

Parkreiner commentedSep 8, 2025•
edited
Loading

Edit: Nvm. Got it figured out. PR should be ready to go

container:{
flex:1,
// Fallback to 100vh
height:["100vh","-webkit-fill-available"],
Copy link
MemberAuthor

@ParkreinerParkreinerSep 8, 2025•
edited
Loading

Choose a reason for hiding this comment

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

-webkit-fill-available was turned intoheight: stretch. The newh-[stretch,100lvh] class should be fully equivalent

@codercoder deleted a comment fromblink-sobotSep 8, 2025
Comment on lines +1 to +7
declare module"react"{
interfaceCSSProperties{
[key: `--${string}`]:string|number|undefined;
}
}

export{};
Copy link
Member

Choose a reason for hiding this comment

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

omg thank you

css={{flexBasis:width,flexGrow:1}}
className="shrink-0"
className="shrink-0 grow"
style={{flexBasis:width}}
Copy link
Member

Choose a reason for hiding this comment

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

yeah, this always should've been astyle. good catch!

key={option.value}
className={cn(
"data-[disabled]:bg-content-disabled data-[disabled]:text-surface-tertiarydata-[disabled]:hover:bg-content-disabled",
"data-[disabled]:bg-content-disabled data-[disabled]:text-surface-tertiary data-[disabled]:hover:bg-content-disabled",
Copy link
Member

Choose a reason for hiding this comment

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

I love Tailwind

interfaceSearchPropsextendsOmit<BoxProps,"ref">{
$$ref?:Ref<unknown>;
interfaceSearchPropsextendsOmit<HTMLAttributes<HTMLDivElement>,"ref">{
$$ref?:Ref<HTMLDivElement>;
Copy link
Member

Choose a reason for hiding this comment

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

I can't tell if I hate this$$ref thing or think that it's very clever and we should use it everywhere to avoidforwardRef

Copy link
Member

Choose a reason for hiding this comment

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

btw, react 19 now. 👀 we no longer need toOmit here.

/**
* Reusable styles for consumers of the base components
*/
exportconstsearchStyles={
Copy link
Member

Choose a reason for hiding this comment

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

very nice. this should've been avariant prop or something if we actually cared about reuse. I like going with the simplest solution tho.

constinputRef=useRef<HTMLInputElement>(null);

useEffect(()=>{
// MUI's autoFocus behavior is wonky. If you set autoFocus=true, the
Copy link
Member

Choose a reason for hiding this comment

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

it seems to just pass it down to the HTML. but the code is hard to follow, so it might be doing something else. if it causes issues tho I guess it's all the same. 🤷‍♀️

// MUI's autoFocus behavior is wonky. If you set autoFocus=true, the
// component will keep getting focus on every single render, even if there
// are other input elements on screen. We want this to be one-time logic
constonMountRef=useEffectEvent((node:HTMLInputElement|null):void=>{
Copy link
Member

Choose a reason for hiding this comment

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

I kinda liked usinguseEffect for this actually. using a function ref feels like a bit of a hack, and function refs aren't nearly as well known as object refs.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I'm not sure if I understand how ref callbacks are a hack. It's been officially documented behavior for ages, and they just got even more support in React 19 by being allowed to return their own cleanup functions

The other benefit is that you're guaranteed that the logic will fire when the React node mounts, because the callback is attached directly to the node. WithuseEffect, it's possible to whiff, depending on what kinds of rendering you're doing

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm just really used to ref objects at this point, but yeah I looking into it more it does seem like this is a case they care about supporting.

I'm not sure we're getting any benefit fromuseEffectEvent, but I've come around on the function ref idea.

Copy link
MemberAuthor

@ParkreinerParkreinerSep 17, 2025•
edited
Loading

Choose a reason for hiding this comment

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

We need to stabilize the function reference somehow – ref callbacks are always called every time the function reference changes

I thought thatuseEffectEvent would the easiest way to ensure that the function stays stable, but retains access to the most recent variables in the outer scope. But I just double-checked React's lifecycles, and ref callbacks are called before layout effects (which is what our polyfill is using), so the data will sometimes be stale. Itjust happens to work right now, but for correctness, I'll swap this back to a layout effect

inputRef.current?.focus();
node?.focus();
}
});
Copy link
Member

Choose a reason for hiding this comment

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

was the bug before just that this is missing the deps list?

Suggested change
});
},[]);

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, this was running on each render because there was no list

return<divrole="tablist"className="flex items-baseline"{...props}/>;
exportconstTabsList:FC<TabsListProps>=({
className,
...delegatedProps
Copy link
Member

Choose a reason for hiding this comment

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

I usually like to name this sort of catch allattrs, which is a bit shorter but gets the point across. I've never seendelegated used in this context before.

Copy link
MemberAuthor

@ParkreinerParkreinerSep 15, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Meanwhile I'm not a big fan of any variant of "attributes", because that means something specific in HTML, and a lot of components have non-standard props that can't be safely serialized as HTML values

I'm fine with splitting the difference, and just calling thisprops, though

(a,b)=>
newDate(a.created_at).getTime()-newDate(b.created_at).getTime(),
typeBuildStatsItemProps=Readonly<
PropsWithChildren<{
Copy link
Member

Choose a reason for hiding this comment

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

don't usePropsWithChildren, just usechildren?: React.ReactNode

syncParentSize();
constresizeObserver=newResizeObserver(syncParentSize);
resizeObserver.observe(document.body);
return()=>resizeObserver.disconnect();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return()=>resizeObserver.disconnect();
return()=>{
resizeObserver.disconnect();
}

interfaceSearchPropsextendsOmit<BoxProps,"ref">{
$$ref?:Ref<unknown>;
interfaceSearchPropsextendsOmit<HTMLAttributes<HTMLDivElement>,"ref">{
$$ref?:Ref<HTMLDivElement>;
Copy link
Member

Choose a reason for hiding this comment

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

btw, react 19 now. 👀 we no longer need toOmit here.


typeSearchInputProps=InputHTMLAttributes<HTMLInputElement>&{
label?:string;
$$ref?:Ref<HTMLInputElement>;
Copy link
Member

Choose a reason for hiding this comment

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

same here, no need for$$ref

// MUI's autoFocus behavior is wonky. If you set autoFocus=true, the
// component will keep getting focus on every single render, even if there
// are other input elements on screen. We want this to be one-time logic
constonMountRef=useEffectEvent((node:HTMLInputElement|null):void=>{
Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm just really used to ref objects at this point, but yeah I looking into it more it does seem like this is a case they care about supporting.

I'm not sure we're getting any benefit fromuseEffectEvent, but I've come around on the function ref idea.


exportconstLOGS_TAB_KEY="logs";

constsortLogsByCreatedAt=(logs:ProvisionerJobLog[])=>{
Copy link
Member

Choose a reason for hiding this comment

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

I kinda liked having this as a function with a name

interfaceSearchPropsextendsOmit<BoxProps,"ref">{
$$ref?:Ref<unknown>;
interfaceSearchPropsextendsHTMLAttributes<HTMLDivElement>{
ref?:RefObject<HTMLDivElement>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ref?:RefObject<HTMLDivElement>;
ref?:Ref<HTMLDivElement>;

resizeObserver.disconnect();
};

syncParentSize();
Copy link
Member

Choose a reason for hiding this comment

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

I don't really see why we even need to involve the state cycle here. why not justcontentEl.style.height = contentEl.parentElement.clientHeight?

@ParkreinerParkreiner merged commitd3bf506 intomainSep 17, 2025
31 checks passed
@ParkreinerParkreiner deleted the mes/emo-01c branchSeptember 17, 2025 22:55
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsSep 17, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@aslilacaslilacaslilac approved these changes

Assignees

@ParkreinerParkreiner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@Parkreiner@aslilac
[8]ページ先頭
©2009-2025 Movatter.jp