[FEAT] Optimized Integration section for mobile view #56
Conversation
|
@cloudsamrajya is attempting to deploy a commit to the idan lodzki's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughImplemented mobile-responsive behavior in the IntegrationsSection: detect mobile viewport, show only the first four integrations by default on small screens, and add a "View more"/"View less" toggle to expand/collapse the full list; updated integration card rendering and link styling. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
Comment |
|
Thanks for your contribution! Join our Slack: https://join.slack.com/t/opsimate/shared_invite/zt-39bq3x6et-NrVCZzH7xuBGIXmOjJM7gA Please make sure to include an image with your PR — it really helps us review and understand the changes better. Only in rare cases will we accept a PR without one. Also, take a moment to review your code to ensure it’s clear, readable, and easy to follow. PRs from contributors who haven’t joined our Slack community or starred the repository won’t be reviewed — we’d love to see you join and be part of the project! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/IntegrationsSection.tsx (1)
291-291: Remove unnecessary blank lines.Extra blank lines at lines 291, 297, and 301 create inconsistent spacing. Consider removing them for cleaner formatting.
Also applies to: 297-297, 301-301
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/IntegrationsSection.tsx(4 hunks)
🔇 Additional comments (4)
components/IntegrationsSection.tsx (4)
1-9: LGTM!The imports are appropriate for the new mobile optimization functionality.
242-245: Verify unrelated SVG path modification.The Systemd icon's SVG path was modified, but this change appears unrelated to the mobile optimization feature. Please confirm this modification is intentional.
259-259: LGTM!The conditional rendering logic correctly displays the first 4 integrations on mobile by default and all integrations when
showAllis toggled or on desktop.Also applies to: 282-282
281-281: LGTM!The responsive gap adjustment (
gap-3 md:gap-6) correctly reduces spacing on mobile as per the PR objectives.
| const [isMobile, setIsMobile] = useState(false); | ||
| const [showAll, setShowAll] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| const checkScreen = () => setIsMobile(window.innerWidth < 768); | ||
| checkScreen(); | ||
| window.addEventListener("resize", checkScreen); | ||
| return () => window.removeEventListener("resize", checkScreen); | ||
| }, []); |
There was a problem hiding this comment.
Address SSR hydration mismatch and optimize resize handler.
Two issues to address:
-
SSR Hydration Mismatch: Initializing
isMobileasfalsecan cause a hydration mismatch in SSR environments (Next.js). The server renders the desktop view, but if the client is mobile, it flashes desktop content before switching to mobile. -
Performance: The resize listener fires on every pixel change, causing unnecessary re-renders even when the viewport doesn't cross the 768px threshold.
Apply this diff to fix both issues:
- const [isMobile, setIsMobile] = useState(false);
+ const [isMobile, setIsMobile] = useState(() => {
+ if (typeof window !== 'undefined') {
+ return window.innerWidth < 768;
+ }
+ return false;
+ });
const [showAll, setShowAll] = useState(false);
useEffect(() => {
- const checkScreen = () => setIsMobile(window.innerWidth < 768);
+ const checkScreen = () => {
+ const mobile = window.innerWidth < 768;
+ setIsMobile(prev => prev !== mobile ? mobile : prev);
+ };
checkScreen();
window.addEventListener("resize", checkScreen);
return () => window.removeEventListener("resize", checkScreen);
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [isMobile, setIsMobile] = useState(false); | |
| const [showAll, setShowAll] = useState(false); | |
| useEffect(() => { | |
| const checkScreen = () => setIsMobile(window.innerWidth < 768); | |
| checkScreen(); | |
| window.addEventListener("resize", checkScreen); | |
| return () => window.removeEventListener("resize", checkScreen); | |
| }, []); | |
| const [isMobile, setIsMobile] = useState(() => { | |
| if (typeof window !== 'undefined') { | |
| return window.innerWidth < 768; | |
| } | |
| return false; | |
| }); | |
| const [showAll, setShowAll] = useState(false); | |
| useEffect(() => { | |
| const checkScreen = () => { | |
| const mobile = window.innerWidth < 768; | |
| setIsMobile(prev => prev !== mobile ? mobile : prev); | |
| }; | |
| checkScreen(); | |
| window.addEventListener("resize", checkScreen); | |
| return () => window.removeEventListener("resize", checkScreen); | |
| }, []); |
components/IntegrationsSection.tsx
Outdated
| {isMobile && integrations.length > 4 && ( | ||
| <button | ||
| className="mt-2 text-blue-600 hover:text-blue-700 text-sm font-medium " | ||
| onClick={() => setShowAll(!showAll)} | ||
| > | ||
| {!showAll ? "View more" : "View less"} | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
Move button outside grid and add accessibility attributes.
Two issues:
-
Incorrect Placement: The button is inside the grid, making it render as a grid item. This creates awkward layout where the button appears in a card-like position.
-
Missing Accessibility: The button lacks
aria-expandedto indicate its state to screen readers.
Apply this diff:
- {/* Integrations Grid */}
- <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-3 md:gap-6 mb-16">
+ {/* Integrations Grid */}
+ <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-3 md:gap-6">
{items.map((integration, index) => (
<div
key={index}
className="bg-surface-50 dark:bg-surface-800 border border-surface-200 dark:border-surface-700 rounded-xl p-6 hover:shadow-lg dark:hover:shadow-white/10 hover:border-blue-200 dark:hover:border-gray-300 transition-all duration-300 group"
>
{/* Integration Icon */}
<div className="w-12 h-12 bg-surface-200 dark:bg-surface-700 rounded-lg mb-4 flex items-center justify-center group-hover:bg-blue-50 dark:group-hover:bg-gray-600 transition-colors duration-300">
{integration.icon}
</div>
<div className="mb-2 ">
<h3 className="font-semibold text-surface-900 dark:text-surface-100">
{integration.name}
</h3>
</div>
<div className="text-surface-600 dark:text-surface-400 text-sm mb-4 leading-relaxed ">
{integration.description}
</div>
<a
href={integration.link}
target="_blank"
rel="noopener noreferrer"
className="text-blue-600 hover:text-blue-700 text-sm font-medium flex items-center gap-1 transition-colors duration-200"
>
Learn More
<ExternalLink className="h-3 w-3" />
</a>
</div>
))}
- {isMobile && integrations.length > 4 && (
+ </div>
+
+ {/* View More/Less Button */}
+ {isMobile && integrations.length > 4 && (
+ <div className="text-center mt-6 mb-16">
<button
- className="mt-2 text-blue-600 hover:text-blue-700 text-sm font-medium "
+ className="text-blue-600 hover:text-blue-700 text-sm font-medium"
onClick={() => setShowAll(!showAll)}
+ aria-expanded={showAll}
+ aria-controls="integrations-grid"
>
{!showAll ? "View more" : "View less"}
</button>
- )}
- </div>
+ </div>
+ )}
</div>
</section>Also add an id to the grid:
- <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-3 md:gap-6">
+ <div id="integrations-grid" className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-3 md:gap-6">Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/IntegrationsSection.tsx around lines 313 to 320, the "View
more/View less" button is currently rendered inside the grid (making it a grid
item) and lacks accessibility attributes; move the button markup so it sits
immediately after the grid container (outside the element that has the grid
classes) so it no longer becomes a grid child, add an id attribute to the grid
container (e.g., id="integrations-grid") and update the button to include
aria-expanded={showAll} and aria-controls="integrations-grid" to reflect and
link its state for screen readers.
|
Hey @cloudsamrajya, thanks for your contribution! I noticed two potential issues:
Please try to resolve them following the guidance provided by CodeRabbit's review. |
yjrathod
left a comment
yjrathod
left a comment
There was a problem hiding this comment.
These two suggested changes are needed, Initialize state properly so mobile users don't see a flash of desktop content, Optimize the resize handler properly!
3 participants
Issue Reference
Closes #50
What Was Changed
Screenshots
VID_20251026_062001.1.mp4
Summary by CodeRabbit
New Features
Style