Skip to content

fix: centered card in casestudies page #3805

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

Merged
merged 9 commits into from
Apr 6, 2025
Merged

fix: centered card in casestudies page #3805

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
06ada4d
fixes centered card
cbum-dev Mar 6, 2025
bbcb787
Merge branch 'master' into issues/#3788
asyncapi-bot Mar 6, 2025
f051a96
Update package-lock.json
cbum-dev Mar 6, 2025
08df39e
Update CaseStudyCard.tsx
cbum-dev Mar 8, 2025
f1c8c54
Merge branch 'master' into issues/#3788
sambhavgupta0705 Mar 19, 2025
7f40c5b
Merge branch 'master' into issues/#3788
sambhavgupta0705 Mar 20, 2025
6b1a7b0
Merge branch 'master' into issues/#3788
sambhavgupta0705 Apr 6, 2025
eeab2c2
Merge branch 'master' into issues/#3788
sambhavgupta0705 Apr 6, 2025
71c818c
Merge branch 'master' into issues/#3788
sambhavgupta0705 Apr 6, 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
4 changes: 2 additions & 2 deletions components/CaseStudyCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ export default function CaseStudyCard({ studies = [] }: ICaseStudyCardProps) {
}

return (
<div className='flex flex-wrap gap-3 pt-10 lg:grid lg:grid-cols-3 lg:gap-8 lg:text-center'>
<div className='flex flex-wrap gap-3 pt-10 lg:flex lg:items-center lg:justify-center lg:gap-8 lg:text-center'>
Copy link
Member

@akshatnema akshatnema Mar 8, 2025

Choose a reason for hiding this comment

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

Since we are using flex for all viewports, can't we simplify this to use flex in all viewports?

@coderabbitai can you suggest something here?

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 8, 2025

Choose a reason for hiding this comment

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

@akshatnema You're absolutely right! Since we're using flex layout for all viewports now, we can simplify this by removing the redundant lg:flex. Here's a simplified version:

<div className='flex flex-wrap gap-3 pt-10 lg:items-center lg:justify-center lg:gap-8 lg:text-center'>

This removes the duplicate flex declaration while preserving all the functionality.

If you want items centered on all viewports (not just large screens), you could further simplify to:

<div className='flex flex-wrap items-center justify-center gap-3 pt-10 lg:gap-8 lg:text-center'>

This would depend on the desired mobile layout - does it need center alignment everywhere or just on larger screens?

Copy link
Member

@akshatnema akshatnema Mar 8, 2025

Choose a reason for hiding this comment

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

@cbum-dev Can you please make this change?

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 8, 2025

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

{studies.map((study, index) => (
<a key={index} href={`casestudies/${study.id}`}>
<div
className='h-full min-h-[300px] max-w-sm overflow-hidden rounded-md border border-gray-200 bg-white p-4'
className='h-full min-h-[300px] max-w-sm overflow-hidden lg:w-[350px] rounded-md border border-gray-200 bg-white p-4'
Copy link
Member

@akshatnema akshatnema Mar 8, 2025

Choose a reason for hiding this comment

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

@coderabbitai Can we do this somewhat better instead of hardcoding it?

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 8, 2025

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for case study implementations
echo "=== Searching for case study implementations ==="
rg -A 5 -B 5 "CaseStudyCard" --type tsx --type jsx

# Search for similar card components in the codebase
echo "=== Searching for other card components ==="
fd "Card.tsx" --type f

# Look at the issue this PR is fixing
echo "=== Checking for the issue reference in PR description ==="
gh pr view 3805 --json body | jq -r '.body'

# Check for other responsive card layouts in the codebase
echo "=== Looking for responsive width patterns in other card components ==="
rg "className=.*w-\[" --type tsx --type jsx

Length of output: 2201

🏁 Script executed:

#!/bin/bash
# Search for case study implementations with correct file types
echo "=== Searching for case study implementations ==="
rg "CaseStudyCard" --type-not md

# Check issue #3788 which this PR fixes
echo "=== Checking issue #3788 ==="
gh issue view 3788 --json title,body | jq -r '"\(.title)\n\n\(.body)"'

# Looking for responsive width patterns in other components
echo "=== Looking for responsive width patterns ==="
rg "className.*w-\[" --type-not md

# Check similar responsive design patterns in other card components
echo "=== Examining responsive design in other card components ==="
fd "Card.tsx" -e tsx -e jsx --exec cat {} | head -n 50

Length of output: 7924

@akshatnema Yes, we can improve this implementation by avoiding the hardcoded width. Instead of using lg:w-[350px], here are some better alternatives:

  1. Use a relative width with flex-basis:
className='h-full min-h-[300px] max-w-sm overflow-hidden lg:basis-1/3 lg:max-w-[350px] rounded-md border border-gray-200 bg-white p-4'
  1. Use a percentage-based width that will scale with the container:
className='h-full min-h-[300px] max-w-sm overflow-hidden lg:w-[30%] rounded-md border border-gray-200 bg-white p-4'
  1. Let the content determine the width with flex-grow and constraints:
className='h-full min-h-[300px] max-w-sm overflow-hidden lg:flex-grow lg:max-w-[350px] rounded-md border border-gray-200 bg-white p-4'

Any of these approaches would be more adaptable to different screen sizes while still maintaining the centered layout that was needed to fix the original issue.

Copy link
Contributor Author

@cbum-dev cbum-dev Mar 8, 2025

Choose a reason for hiding this comment

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

@coderabbitai Can we do this somewhat better instead of hardcoding it?

Ok let me check which one works best.

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 8, 2025

Choose a reason for hiding this comment

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

@cbum-dev Great! Looking forward to seeing which approach works best for your use case. Let me know if you need any further assistance with implementing these alternatives.

Copy link
Collaborator

@TRohit20 TRohit20 Mar 17, 2025

Choose a reason for hiding this comment

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

@akshatnema @cbum-dev In this case, Instead of keeping it Static/Hardcoded, the optimal way is to make it dynamic using percentages, this way based on the user screen size, it will dynamically adapt and be responsive per user screen.

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 17, 2025

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

data-testid='CaseStudyCard-main'
>
<span className='mr-2'>
Expand Down
Loading