Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: migrate ImagePickerPopover to Propel Tabs component and render only enabled tabs#8290

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
prateekshourya29 wants to merge1 commit intopreview
base:preview
Choose a base branch
Loading
fromfix-image-picker

Conversation

@prateekshourya29
Copy link
Member

@prateekshourya29prateekshourya29 commentedDec 10, 2025
edited by coderabbitaibot
Loading

Description

  • Replace custom tab implementation with Propel Tabs
  • Dynamically render only enabled tabs based on configuration
  • Filter tabs by isEnabled property for cleaner conditional rendering
  • Improve tab navigation and accessibility with Propel components

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Code refactoring

Summary by CodeRabbit

  • Refactor

    • Updated image picker popover component with improved internal structure for better usability.
    • Enhanced tab navigation system with dynamic tab rendering.
  • Style

    • Increased container height for better visual spacing in the image picker interface.
    • Minor update to upload status messaging.

✏️ Tip: You can customize this high-level summary in your review settings.

…nly enabled tabs- Replace custom tab implementation with Propel Tabs- Dynamically render only enabled tabs based on configuration- Filter tabs by isEnabled property for cleaner conditional rendering- Improve tab navigation and accessibility with Propel components
@coderabbitai
Copy link
Contributor

coderabbitaibot commentedDec 10, 2025
edited
Loading

Walkthrough

The image-picker-popover component replaces Headless UI Tab components with a custom Tabs implementation from@plane/propel/tabs. The migration includes restructuring the tab rendering flow, introducing dynamic tab enabling logic, adjusting container height from md:h-[28rem] to md:h-[36rem], reorganizing tab content into separate Tabs.Content blocks, and updating the upload button label text from "Uploading..." to "Uploading".

Changes

Cohort / File(s)Summary
Tab library migration & dynamic enabling
apps/web/core/components/core/image-picker-popover.tsx
Replaced Headless UI Tab.Group/Tab.List/Tab/Tab.Panels/Tab.Panel with@plane/propel/tabs (Tabs, Tabs.List, Tabs.Trigger, Tabs.Indicator, Tabs.Content). Added computed enabledTabs filtering. Updated imports to remove Headless UI Tab and add@plane/propel/tabs. Reorganized unsplash, images, and upload panels into separate Tabs.Content blocks with explicit value-based rendering.
Layout & styling adjustments
apps/web/core/components/core/image-picker-popover.tsx
Updated container height from md:h-[28rem] to md:h-[36rem]. Adjusted wrapper structure to accommodate new Tabs layout.
Content & button text refinements
apps/web/core/components/core/image-picker-popover.tsx
Changed upload button label from "Uploading..." to "Uploading". Updated tab trigger rendering and content placement to reflect new Tabs component structure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas requiring extra attention:
    • Verify that dynamic tab enabling viaenabledTabs computation correctly filters and displays only enabled tabs
    • Confirm layout height change (md:h-[28rem] → md:h-[36rem]) maintains proper visual alignment and scroll behavior
    • Validate that all tab content (unsplash, images, upload) renders correctly under the new Tabs.Content structure
    • Check that tab selection and navigation behavior works as expected with the new@plane/propel/tabs implementation

Poem

🐰 Tabs were shuffled, new library in sight,
From Headless UI's grip to Propel's bright light,
Dynamic tabs now dance, enabled with care,
A taller container breathes cleaner air,
Upload buttons whisper softly, "uploading" they say! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the main changes: migrating to Propel Tabs and implementing dynamic tab rendering based on enabled state.
Description check✅ PassedThe description covers the main changes and includes the Type of Change section, but lacks detailed test scenarios, screenshots, and references as specified in the template.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branchfix-image-picker

Thanks for usingCodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment@coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitaicoderabbitaibot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/web/core/components/core/image-picker-popover.tsx (1)

213-280:Consider conditionally renderingTabs.Content for disabled tabs.

TheTabs.Trigger components are correctly filtered to show only enabled tabs, but allTabs.Content sections are always rendered regardless ofisEnabled status. WhenhasUnsplashConfigured isfalse, the "unsplash" tab trigger won't exist, but this content block is still in the DOM.

For consistency with the PR objective of rendering only enabled tabs, consider wrapping thisTabs.Content conditionally:

+              {hasUnsplashConfigured && (                <Tabs.Content value="unsplash" className="h-full w-full space-y-4">                  {/* ... content ... */}                </Tabs.Content>+              )}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between20510bb ande4ccda3.

📒 Files selected for processing (1)
  • apps/web/core/components/core/image-picker-popover.tsx (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

**/*.{ts,tsx,mts,cts}: Useconst type parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfies operator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitis return types in filter/check functions
UseNoInfer<T> utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true) blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusing declarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" } for import attributes; avoid deprecatedassert syntax (TypeScript 5.3/5.8+)
Useimport type explicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntax flag
Use.ts,.mts,.cts extensions inimport type statements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" } for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSet methods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy /Map.groupBy standard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers() for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuper in classes (TypeScript 5....

Files:

  • apps/web/core/components/core/image-picker-popover.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T20:42:31.843ZLearning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.

Applied to files:

  • apps/web/core/components/core/image-picker-popover.tsx
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7989File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46Timestamp: 2025-10-21T17:22:05.204ZLearning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.

Applied to files:

  • apps/web/core/components/core/image-picker-popover.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Build and lint web apps
🔇 Additional comments (6)
apps/web/core/components/core/image-picker-popover.tsx (6)

8-13:LGTM!

The import changes correctly reflect the migration from Headless UI Tab to Propel Tabs, while preserving Popover from Headless UI.


25-29:LGTM!

Clean type definition that supports the dynamic tab enablement feature.


63-84:LGTM!

The tab filtering logic is well-structured. The memoization dependencies are correct, and the fallback to"images" in thedefaultValue prop (line 203) provides a safe default ifenabledTabs[0] were ever undefined.


203-211:LGTM!

The Tabs structure withdefaultValue fallback is well-implemented. The dynamic rendering of only enabled tabs inTabs.List correctly implements the PR objective.


354-362:LGTM!

The upload button implementation is clean. The text change from "Uploading..." to "Uploading" works well with theloading prop which likely provides a visual loading indicator, making the ellipsis redundant.


201-211:Propel Tabs component provides expected accessibility features through Base UI Components.

The@plane/propel/tabs component wraps@base-ui-components/react/tabs (v1.0.0-beta.3), a WCAG 2.1 Level AA-compliant headless component library. Base UI automatically provides all required ARIA attributes (role="tablist",role="tab",aria-selected, etc.) and keyboard navigation support (Arrow keys, Home/End). The propel wrapper properly delegates props without overriding these accessibility features.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@coderabbitaicoderabbitai[bot]coderabbitai[bot] left review comments

@sriramveeraghantasriramveeraghantaAwaiting requested review from sriramveeraghanta

At least 1 approving review is required to merge this pull request.

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@prateekshourya29

[8]ページ先頭

©2009-2025 Movatter.jp