Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

update(frontend) Featured agents cards #9482

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

update(frontend) Featured agents cards #9482

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6e64e4
Fix(Frontend) Update Featured agents cards
Bentlybro Feb 14, 2025
798e194
prettier
Bentlybro Feb 14, 2025
9cb96ce
Merge branch 'dev' into update-Featured-agents-cards
Bentlybro Feb 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions autogpt_platform/frontend/src/components/agptui/FeaturedAgentCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,20 @@ export const FeaturedAgentCard: React.FC<FeaturedStoreCardProps> = ({
data-testid="featured-store-card"
onMouseEnter={() => setIsHovered(true)}
onMouseLeave={() => setIsHovered(false)}
className={backgroundColor}
className={`flex h-[482px] w-[440px] flex-col ${backgroundColor}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid hardcoding dimensions on the Card component.

The card should flow naturally based on its content, with layout constraints handled by its parent container (the CarouselItem from the FeatureAgentSection in this case).

>
<CardHeader>
<CardTitle>{agent.agent_name}</CardTitle>
<CardDescription>{agent.description}</CardDescription>
<CardHeader className="flex-none space-y-3 pb-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the base CardHeader clean it already has good default spacing.

Adding flex-none and custom spacing here makes the component less reusable. If we need consistent heights, we can handle that at the carousel layout level.

<CardTitle className="text-xl leading-tight">
Copy link
Contributor

Choose a reason for hiding this comment

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

You have a good approach on adding a lineclamp for the description.

Since we are not too sure on how long these titles can be lets add a line-clamp-2 here as well to be safe.

Suggested change
<CardTitle className="text-xl leading-tight">
<CardTitle className="line-clamp-2 text-base sm:text-xl">

{agent.agent_name}
</CardTitle>
<CardDescription className="text-sm">
By {agent.creator}
</CardDescription>
</CardHeader>
<CardContent>
<div className="relative h-[397px] w-full overflow-hidden rounded-xl">
<CardContent className="relative flex-1 p-4">
<div className="absolute inset-0 m-4 overflow-hidden rounded-xl">
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets consider using aspect ratios instead of absolute positioning its easier to predict how it will change with a responsive design.

I'd suggest trying something more towards:

Suggested change
<div className="absolute inset-0 m-4 overflow-hidden rounded-xl">
<div className="relative aspect-[16/9] sm:aspect-[4/3] overflow-hidden rounded-xl">

<div
className={`transition-opacity duration-200 ${isHovered ? "opacity-0" : "opacity-100"}`}
className={`h-full w-full transition-opacity duration-200 ${isHovered ? "opacity-0" : "opacity-100"}`}
>
<Image
src={agent.agent_image || "/AUTOgpt_Logo_dark.png"}
Expand All @@ -47,17 +51,17 @@ export const FeaturedAgentCard: React.FC<FeaturedStoreCardProps> = ({
/>
</div>
<div
className={`absolute inset-0 overflow-y-auto transition-opacity duration-200 ${
className={`absolute inset-0 overflow-y-auto p-4 transition-opacity duration-200 ${
isHovered ? "opacity-100" : "opacity-0"
} rounded-xl dark:bg-neutral-700`}
}`}
>
<p className="text-base text-neutral-800 dark:text-neutral-200">
<CardDescription className="line-clamp-[11] text-sm leading-relaxed text-muted-foreground">
Copy link
Contributor

Choose a reason for hiding this comment

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

We will want to keep mobile in mind when building out specialized components like this.
It would be good to add a something similar to this:

Suggested change
<CardDescription className="line-clamp-[11] text-sm leading-relaxed text-muted-foreground">
<CardDescription className="line-clamp-[6] sm:line-clamp-[8] text-xs sm:text-sm">
{agent.description}
</CardDescription>

{agent.description}
</p>
</CardDescription>
</div>
</div>
</CardContent>
<CardFooter className="flex items-center justify-between">
<CardFooter className="flex h-[60px] flex-none items-center justify-between">
Copy link
Contributor

Choose a reason for hiding this comment

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

The CardFooter component already provides good spacing and alignment. We can remove the custom height and let it flow naturally:

Suggested change
<CardFooter className="flex h-[60px] flex-none items-center justify-between">
<CardFooter className="flex justify-between">

<div className="font-semibold">
{agent.runs?.toLocaleString() ?? "0"} runs
</div>
Expand Down
Loading