update(frontend) Featured agents cards #9482
Conversation
Bentlybro
requested review from
kcze and
majdyz
and removed request for
a team
github-actions
bot
added
platform/frontend
|
Here's the code health analysis summary for commits Analysis Summary
|
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
✅ Deploy Preview for auto-gpt-docs canceled.
|
Bentlybro
changed the title
There was a problem hiding this comment.
Great job on this component I like where this is headed.
I have a few suggestions to help make this more maintainable and mobile-friendly:
-
I notice you're adding quite a few styles to control the card heights and layout. Instead of managing this at the card level, we should try to handle consistency through the carousel layout. This will make our components more reusable.
-
Some helpful things to consider:
- The
FeaturedAgentCardshould be as clean as possible, with minimal style overrides - Let try using aspect ratios instead of fixed heights for responsive images
- The
Feel free to reach out to me if you would like to sync up at all.
I'd be happy to show you how we can achieve the same layout consistency while keeping our base components clean and reusable.
| <CardHeader> | ||
| <CardTitle>{agent.agent_name}</CardTitle> | ||
| <CardDescription>{agent.description}</CardDescription> | ||
| <CardHeader className="flex-none space-y-3 pb-2"> |
There was a problem hiding this comment.
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.
| <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"> |
There was a problem hiding this comment.
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:
| <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"> |
| @@ -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}`} | |||
There was a problem hiding this comment.
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).
| <CardTitle>{agent.agent_name}</CardTitle> | ||
| <CardDescription>{agent.description}</CardDescription> | ||
| <CardHeader className="flex-none space-y-3 pb-2"> | ||
| <CardTitle className="text-xl leading-tight"> |
There was a problem hiding this comment.
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.
| <CardTitle className="text-xl leading-tight"> | |
| <CardTitle className="line-clamp-2 text-base sm:text-xl"> |
| > | ||
| <p className="text-base text-neutral-800 dark:text-neutral-200"> | ||
| <CardDescription className="line-clamp-[11] text-sm leading-relaxed text-muted-foreground"> |
There was a problem hiding this comment.
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:
| <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> |
| </div> | ||
| </div> | ||
| </CardContent> | ||
| <CardFooter className="flex items-center justify-between"> | ||
| <CardFooter className="flex h-[60px] flex-none items-center justify-between"> |
There was a problem hiding this comment.
The CardFooter component already provides good spacing and alignment. We can remove the custom height and let it flow naturally:
| <CardFooter className="flex h-[60px] flex-none items-center justify-between"> | |
| <CardFooter className="flex justify-between"> |
Changes 🏗️
right now the featured agents cards dont have a set width/height, so they change depending on the content, and the description is duplicated. also, the "by {creator username}" was removed. currently it looks like this:
with my changes, i set a fixed width and height so they dont change, the description now only shows on hover and is truncated to 11 lines, and i added back the "by {creator username}"