fix: centered card in casestudies page #3805
Conversation
cbum-dev
requested review from
derberg,
akshatnema,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe changes update the layout in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
asyncapi-bot
requested review from
devilkiller-ag,
sambhavgupta0705,
Aahil13 and
VaishnaviNandakumar
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3805 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 667 667
Branches 113 113
=========================================
Hits 667 667 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
components/CaseStudyCard.tsx
Outdated
| @@ -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'> | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@cbum-dev Can you please make this change?
There was a problem hiding this comment.
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!
components/CaseStudyCard.tsx
Outdated
| {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' |
There was a problem hiding this comment.
@coderabbitai Can we do this somewhat better instead of hardcoding it?
There was a problem hiding this comment.
🧩 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:
- 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'- 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'- 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.
There was a problem hiding this comment.
@coderabbitai Can we do this somewhat better instead of hardcoding it?
Ok let me check which one works best.
There was a problem hiding this comment.
@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.
|
@cbum-dev it is maintainer's task to resolve the threads as by this we keep a track of what comments we have added or not |
|
Sorry @sambhavgupta0705 I'll keep that in mind. |
|
Kindly mark them as unresolved |
Description
-fixes #3788
Screenshots
Case1: When items is more than 2
Case1: When items is less than or equal to 2
Summary by CodeRabbit