feat(view): Collapsible section #2743
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdjusts SingleView layout and derives the primary title from data; adds collapsible behavior to view sections with local expand/collapse state, interactive and keyboard-accessible headers, chevron indicator, conditional loading/error UI, and gated rendering of View content. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)src/pages/views/components/ViewSection.tsx (1)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pages/views/components/ViewSection.tsx (1)
45-61: Extract duplicated header into a reusable component.The header rendering logic is duplicated between the error case (lines 45-61) and success case (lines 73-87). This violates DRY principles and makes maintenance harder.
Consider extracting the header into a separate component or at least into a helper function to eliminate duplication:
const CollapsibleHeader: React.FC<{ isExpanded: boolean; onToggle: () => void; icon?: string; title: string; }> = ({ isExpanded, onToggle, icon, title }) => ( <div className="mb-2 flex cursor-pointer items-center gap-2 rounded px-2 py-1" role="button" tabIndex={0} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onToggle(); } }} onClick={onToggle} > <IoChevronDownOutline className={`h-4 w-4 flex-shrink-0 transform text-gray-600 transition-transform ${!isExpanded ? "-rotate-90" : ""}`} /> {icon && ( <Icon name={icon} className="h-5 w-5 flex-shrink-0 text-gray-600" /> )} <h3 className="text-lg font-semibold text-gray-600">{title}</h3> </div> );Also applies to: 73-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/views/components/SingleView.tsx(4 hunks)src/pages/views/components/ViewSection.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/views/components/ViewSection.tsx (1)
src/api/services/views.ts (1)
View(7-19)
🔇 Additional comments (7)
src/pages/views/components/ViewSection.tsx (3)
1-1: LGTM: New imports and state initialization.The
useStatehook andIoChevronDownOutlineicon imports are correctly added, and initializing sections to expanded (true) provides good default UX.Also applies to: 3-3, 20-20
62-66: Verify: Error visibility when section is collapsed.Error messages are only displayed when the section is expanded. While sections start expanded by default, if a user collapses a section with an error, that error becomes hidden. This could prevent users from noticing critical issues.
Consider these alternatives:
- Add a visual indicator (e.g., red color, warning icon) to the header when an error exists, even when collapsed
- Always show errors regardless of expansion state
- Keep current behavior if hiding errors is intentional for UX reasons
Which approach aligns best with the intended user experience?
88-108: LGTM: Conditional rendering with loading state.The conditional rendering logic correctly handles both loading and loaded states when expanded. The loading skeleton provides good UX feedback, and passing an empty string for
title(line 96) is appropriate since the title is already displayed in the section header.src/pages/views/components/SingleView.tsx (4)
166-166: LGTM: Primary section now has meaningful title.Changing the primary view section title from an empty string to
title || nameensures the collapsible header displays a meaningful title, consistent with how additional sections are handled.
188-188: LGTM: Padding adjustment aligns with collapsible sections.The change from
p-6 pb-0topx-6removes vertical padding, which is appropriate since the collapsible sections now manage their own spacing internally. Horizontal padding is preserved for proper content alignment.
204-204: LGTM: Top margin added for primary section.Adding
mt-2provides appropriate spacing above the primary view section, complementing the padding changes on the parent container.
217-217: LGTM: Reduced section spacing aligns with collapsible design.The spacing reduction from
mt-6 pt-6(48px) tomt-4(16px) is appropriate for the new collapsible sections. Each section now has its own distinct header, reducing the need for excessive vertical separation. This prevents large gaps when sections are collapsed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/views/components/ViewSection.tsx (1)
53-61: Keyboard accessibility properly implemented.The accessibility attributes (
role="button",tabIndex={0},aria-expanded,aria-controls) and keyboard handler have been added as requested in the previous review.
🧹 Nitpick comments (2)
src/pages/views/components/ViewSection.tsx (2)
116-128: Minor inconsistency in optional chaining usage.Line 125 uses
sectionViewResult.requestFingerprintwithout optional chaining while other props use?.. WhilesectionViewResultis guaranteed to exist here, consider using consistent access patterns for maintainability.- requestFingerprint={sectionViewResult.requestFingerprint} + requestFingerprint={sectionViewResult?.requestFingerprint}
88-107: Consider extracting the header into a reusable component.The header rendering (chevron, icon, title) is duplicated between the error path (lines 53-74) and success path (lines 88-107). Extracting to a
SectionHeadercomponent would reduce duplication.const SectionHeader: React.FC<{ section: Section; isExpanded: boolean; onToggle: () => void; onKeyDown: (e: React.KeyboardEvent<HTMLDivElement>) => void; contentId: string; }> = ({ section, isExpanded, onToggle, onKeyDown, contentId }) => ( <div role="button" tabIndex={0} aria-expanded={isExpanded} aria-controls={contentId} className="mb-2 flex cursor-pointer items-center gap-2 rounded px-2 py-1" onClick={onToggle} onKeyDown={onKeyDown} > <IoChevronDownOutline className={`h-4 w-4 flex-shrink-0 transform text-gray-600 transition-transform ${!isExpanded ? "-rotate-90" : ""}`} /> {section.icon && ( <Icon name={section.icon} className="h-5 w-5 flex-shrink-0 text-gray-600" /> )} <h3 className="text-lg font-semibold text-gray-600">{section.title}</h3> </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/views/components/ViewSection.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/views/components/ViewSection.tsx (1)
src/api/services/views.ts (1)
View(7-19)
🔇 Additional comments (2)
src/pages/views/components/ViewSection.tsx (2)
1-20: LGTM!Clean setup for the collapsible state. Starting with sections expanded by default is good UX for first-time viewers.
42-47: LGTM!Keyboard handler correctly implements accessibility best practices - handling both Enter and Space with proper preventDefault to avoid scroll behavior.
❌ Deploy Preview for flanksource-demo-stable failed. Why did it fail? →
|
❌ Deploy Preview for clerk-saas-ui failed. Why did it fail? →
|
resolves: #2742
Summary by CodeRabbit
New Features
Bug Fixes
UI/Style
✏️ Tip: You can customize this high-level summary in your review settings.