Skip to content

Conversation

@code-asher
Copy link
Member

No description provided.

@code-asher code-asher force-pushed the asher/refactor-template-insights branch 4 times, most recently from 6398427 to 665de65 Compare December 19, 2025 01:55
@code-asher code-asher force-pushed the asher/refactor-template-insights branch from 665de65 to 3934031 Compare December 19, 2025 02:14
@code-asher code-asher requested a review from aslilac December 19, 2025 13:18
Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

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

so I think I'm noticing a pattern on this page. there are a ton of components that all have the same "chrome" around them, and it seems to be duplicated across components.

we also want to be able to put various content within that chrome depending on states like loading, errors, etc.

it feels to me like we should just make that chrome a component that takes children and then rather than all of this duplication and manual ifs to try to reuse the jsx, we could just actually have it be reusable

Comment on lines +346 to +353
css={{
display: "flex",
justifyContent: "space-between",
alignItems: "center",
fontSize: 14,
paddingTop: 8,
paddingBottom: 8,
}}
Copy link
Member

Choose a reason for hiding this comment

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

I know these aren't exactly new but I'm gonna bully you into converting these to tailwind (pls and thank you)

Suggested change
css={{
display: "flex",
justifyContent: "space-between",
alignItems: "center",
fontSize: 14,
paddingTop: 8,
paddingBottom: 8,
}}
className="flex justify-between items-center text-sm py-2"


let content: JSX.Element | JSX.Element[];
if (!error && !users) {
content = <Loader css={{ height: "100%" }} />;
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
content = <Loader css={{ height: "100%" }} />;
content = <Loader className="h-full" />;

same here

paddingBottom: 8,
}}
>
<div css={{ display: "flex", alignItems: "center", gap: 12 }}>
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
<div css={{ display: "flex", alignItems: "center", gap: 12 }}>
<div className="flex items-center gap-3">

>
<div css={{ display: "flex", alignItems: "center", gap: 12 }}>
<Avatar fallback={row.username} src={row.avatar_url} />
<div css={{ fontWeight: 500 }}>{row.username}</div>
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 (I don't remember this one off the top of my head)

content = <NoDataAvailable error={error} />;
}
return (
<Panel {...panelProps} css={{ overflowY: "auto" }}>
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

Comment on lines +416 to +423
css={{
display: "flex",
justifyContent: "space-between",
alignItems: "center",
fontSize: 14,
paddingTop: 8,
paddingBottom: 8,
}}
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
css={{
display: "flex",
justifyContent: "space-between",
alignItems: "center",
fontSize: 14,
paddingTop: 8,
paddingBottom: 8,
}}
className="flex justify-between items-center text-sm py-2"

paddingBottom: 8,
}}
>
<div css={{ display: "flex", alignItems: "center", gap: 12 }}>
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
<div css={{ display: "flex", alignItems: "center", gap: 12 }}>
<div className="flex items-center gap-3">

>
<div css={{ display: "flex", alignItems: "center", gap: 12 }}>
<Avatar fallback={row.username} src={row.avatar_url} />
<div css={{ fontWeight: 500 }}>{row.username}</div>
Copy link
Member

Choose a reason for hiding this comment

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

and here

Comment on lines +430 to +434
css={{
color: theme.palette.text.secondary,
fontSize: 13,
textAlign: "right",
}}
Copy link
Member

Choose a reason for hiding this comment

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

and here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants