Enhance: add contentProps to Select component for customizable Popover alignment and offset #155
Conversation
…r alignment and offset Introduced a new prop, contentProps, to the Select component, allowing users to customize the alignment and side offset of the Popover. This change improves flexibility in the component's usage, enabling better control over its presentation in various contexts.
|
|
WalkthroughAdds support for overriding Select popover alignment via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Consumer
participant UI as Select (Component)
participant Pop as PopoverPrimitive.Content
note over UI,Pop #e6f7ff: New: contentProps -> align/side/sideOffset
Dev->>UI: Render <Select contentProps={ align: 'end', side?, sideOffset? }>
UI->>Pop: Render Content with align/side/sideOffset
note right of Pop: Defaults: align='start', sideOffset=4
Dev->>UI: User opens Select
UI->>Pop: Show positioned popover (listbox)
Dev->>UI: User navigates & selects option
UI-->>Pop: Close popover (set aria-expanded=false)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (1)
Comment |
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
jaruesink
temporarily deployed
to
pr-preview
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/components/src/ui/select.tsx (4)
14-14: Avoid React namespace types; import types directly.You’re already importing types from 'react'. For consistency and to avoid relying on the ambient React namespace, also import ComponentPropsWithoutRef and SVGProps and replace React.ComponentPropsWithoutRef/React.SVGProps usages.
Apply:
-import { +import { forwardRef, type Ref, useEffect, useState, type ButtonHTMLAttributes, type ComponentProps, type ComponentType, type RefAttributes, useId, useRef, + type ComponentPropsWithoutRef, + type SVGProps, } from 'react';And later:
- SearchInput?: ComponentType<React.ComponentPropsWithoutRef<typeof CommandInput>>; - CheckIcon?: React.ComponentType<React.SVGProps<SVGSVGElement>>; - ChevronIcon?: React.ComponentType<React.SVGProps<SVGSVGElement>>; + SearchInput?: ComponentType<ComponentPropsWithoutRef<typeof CommandInput>>; + CheckIcon?: React.ComponentType<SVGProps<SVGSVGElement>>; + ChevronIcon?: React.ComponentType<SVGProps<SVGSVGElement>>;
35-36: Scope of SelectContentProps: consider including alignOffset (optional).contentProps focuses on alignment and offset—good. Adding alignOffset covers a common need without broadening the surface too much.
-export type SelectContentProps = Pick<ComponentProps<typeof PopoverPrimitive.Content>, 'align' | 'side' | 'sideOffset'>; +export type SelectContentProps = Pick< + ComponentProps<typeof PopoverPrimitive.Content>, + 'align' | 'alignOffset' | 'side' | 'sideOffset' +>;
142-145: Pass-through defaults are sensible; consider allowing alignOffset passthrough if added.Defaults (align 'start', sideOffset 4) match our DS. If you add alignOffset above, forward it here without a default to rely on Radix’s default of 0.
<PopoverPrimitive.Content ref={popoverRef} align={contentProps?.align ?? 'start'} side={contentProps?.side} sideOffset={contentProps?.sideOffset ?? 4} + alignOffset={contentProps?.alignOffset}
135-137: Placeholder rendering when value is undefined.If value is undefined, the trigger renders empty text instead of the placeholder. Use a null/empty check rather than value !== ''.
- {value !== '' ? (selectedOption?.label ?? value) : placeholder} + {value != null && value !== '' ? (selectedOption?.label ?? value) : placeholder}apps/docs/src/ui/select-alignment.stories.tsx (1)
72-99: Prefer user interactions over dispatching internal events.Dispatching 'cmdk-item-select' couples the test to cmdk internals. Simulate Enter on the active item instead for a more user-centric test.
- const activeItem = document.querySelector('[cmdk-item][aria-selected="true"]') as HTMLElement; - activeItem.dispatchEvent( - new CustomEvent('cmdk-item-select', { - detail: activeItem.getAttribute('data-value'), - bubbles: true, - }), - ); + const activeItem = document.querySelector('[cmdk-item][aria-selected="true"]') as HTMLElement; + activeItem.focus(); + await userEvent.keyboard('{Enter}', { focusTrap: false });.changeset/align-select-popover-end.md (1)
5-5: Expand the note with the new prop/type for consumers.Add a brief usage snippet and mention of SelectContentProps so downstream users discover the API quickly.
-Allow Select popover content alignment overrides through `contentProps` and document right-aligned usage. +Allow Select popover content alignment overrides through `contentProps` and document right-aligned usage. + +### New +- `SelectProps['contentProps']` to configure popover positioning. +- `SelectContentProps` type (align, side, sideOffset[, alignOffset]). + +### Example +```tsx +<Select + options={options} + value={value} + onValueChange={setValue} + contentProps={{ align: 'end', sideOffset: 8 }} +/> +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/align-select-popover-end.md(1 hunks)apps/docs/src/ui/select-alignment.stories.tsx(1 hunks)packages/components/src/ui/select.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
.changeset/**
📄 CodeRabbit inference engine (AGENTS.md)
When changing published packages, add a Changeset before merge
Files:
.changeset/align-select-popover-end.md
packages/components/src/ui/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
packages/components/src/ui/**/*.tsx: Build on @radix-ui components as the foundation for base UI components
Follow the component composition pattern for UI components that accept form integration
Files:
packages/components/src/ui/select.tsx
packages/components/src/ui/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
Base UI components should be named as ComponentName in ui/ directory
Files:
packages/components/src/ui/select.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
**/*.{tsx,ts}: Props interfaces should be named as ComponentNameProps
Form schemas should be named formSchema or componentNameSchema
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
packages/components/src/{remix-hook-form,ui}/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)
Always export both the component and its props type
Files:
packages/components/src/ui/select.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/*.{ts,tsx}: Use package name imports for published packages (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form')
Import from specific entry points (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form/text-field')
Do not use relative imports across packages (e.g., avoid import { TextField } from '../../packages/components/src/remix-hook-form/text-field')
Order imports: 1) external libraries, 2) internal package imports, 3) cross-package imports, 4) type-only imports (grouped separately)
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
{apps,packages}/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{ts,tsx}: Use relative imports within the same package (e.g., import { FormControl } from './form')
Use relative imports for sibling directories (e.g., import { Button } from '../ui/button')
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
packages/components/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
packages/components/src/**/*.{ts,tsx}: Always export both component and props type (e.g., export { ComponentName }; export type { ComponentNameProps };)
Use named exports for components for better tree-shaking (e.g., export const ComponentName = ...; avoid default exports)
Avoid default exports for components
Use tree-shaking friendly exports@lambdacurry/forms source code lives under packages/components/src and is built to dist/
Files:
packages/components/src/ui/select.tsx
{apps,packages}/**/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
{apps,packages}/**/src/**/*.{tsx,ts}: Use kebab-case for component file names (e.g., text-field.tsx, data-table.tsx)
Match component name in PascalCase to the exported component from the file (e.g., text-field.tsx exports TextField)
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
packages/components/src/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Don't import from remix-hook-form package in ui components
Files:
packages/components/src/ui/select.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
Avoid circular dependencies
Indentation is 2 spaces, max line width 120, and single quotes (Biome enforced)
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
{apps/docs,packages/components/src,types}/**/*.{ts,tsx,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Filenames must be kebab-case (e.g., text-field.tsx, data-table-filter/**)
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
{apps/docs,packages/components/src,types}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Components and types use PascalCase identifiers; keep components pure and fully typed (TypeScript + React, ES modules)
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
{apps/docs,packages/components/src}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
{apps/docs,packages/components/src}/**/*.{ts,tsx}: Hooks use camelCase with a use* prefix
Use named exports only
Avoid cross-package relative imports; respect package boundaries and configured exports/TS paths
Organize imports automatically (Biome)
For form-aware components, use Zod schemas, proper messages, Remix fetcher.Form, and show FormMessage errors
Files:
packages/components/src/ui/select.tsxapps/docs/src/ui/select-alignment.stories.tsx
apps/docs/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)
In apps/docs, import from package name instead of relative paths for cross-package dependencies
Tests should use per-story decorators, semantic queries, and three-phase play tests; run yarn test
Files:
apps/docs/src/ui/select-alignment.stories.tsx
apps/docs/**
📄 CodeRabbit inference engine (.cursor/rules/storybook-testing.mdc)
apps/docs/**: Build Storybook static files before running Playwright tests locally.
Install Playwright browsers and system dependencies before running interaction tests.Storybook docs, examples, and UI tests live in apps/docs
Files:
apps/docs/src/ui/select-alignment.stories.tsx
apps/docs/**/*.{stories.tsx,stories.mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Add stories that exercise critical states to support interaction tests and docs
Files:
apps/docs/src/ui/select-alignment.stories.tsx
🧬 Code graph analysis (1)
apps/docs/src/ui/select-alignment.stories.tsx (1)
packages/components/src/ui/select.tsx (1)
Select(63-275)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (3)
packages/components/src/ui/select.tsx (2)
46-46: Public API addition looks good; ensure barrel re‑exports.contentProps on SelectProps aligns with our guidelines. Make sure the package’s index/barrel re‑exports SelectContentProps so consumers can import the type.
72-72: Destructure default: OK.Plumbing contentProps through the component is clean and minimally invasive.
apps/docs/src/ui/select-alignment.stories.tsx (1)
63-70: Assertion against data-align is solid.Good choice to target the portal root via document and assert data-align='end'.
|
Requested by Jake Ruesink. High-level review
Suggestions (optional polish)
Portal testing rule of thumb (to carry forward)
Overall: recommend moving forward with this PR and closing or updating #154 to adopt the same testing pattern. |
…equested by Jake Ruesink)
|
Requested by Jake Ruesink. Added a concise "Testing Portaled UI (Select, Popover, Combobox)" section to our Storybook test Cursor rules to codify the guidance we discussed:
File: .cursor/rules/storybook-testing.mdc |
Introduced a new prop, contentProps, to the Select component, allowing users to customize the alignment and side offset of the Popover. This change improves flexibility in the component's usage, enabling better control over its presentation in various contexts.
Summary by CodeRabbit
New Features
Documentation
Chores