chore: Dialog container into UI step 1#3294
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis set of changes introduces new dialog UI components and documentation into the codebase, migrating internal dashboard usage to a shared UI package. It adds a generic navigable dialog system, updates OpenAPI type definitions for permissions, and enhances the UI package with Radix-based dialog primitives, parts, and documentation examples. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant UI_Package as @unkey/ui
participant DialogContainer
participant NavigableDialogRoot
User->>App: Clicks "Open Dialog" button
App->>UI_Package: Renders DialogContainer or NavigableDialogRoot
UI_Package->>DialogContainer: Displays modal with header, content, footer
UI_Package->>NavigableDialogRoot: Displays navigable dialog with sidebar and content panels
User->>DialogContainer: Inputs, submits, closes dialog
User->>NavigableDialogRoot: Navigates steps, submits, closes dialog
DialogContainer-->>App: Calls onOpenChange callback
NavigableDialogRoot-->>App: Calls onOpenChange callback
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (14)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
MichaelUnkey
changed the title
MichaelUnkey
changed the title
MichaelUnkey
requested review from
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
packages/api/src/openapi.d.ts (1)
1-4: Note: Changes should be made in the OpenAPI source specification.This file is auto-generated and contains a warning not to make direct changes. Ensure these modifications are reflected in the actual OpenAPI specification source file that generates this TypeScript definition.
apps/engineering/content/design/components/dialog/dialog-container.example.tsx (2)
41-43: Consider using appropriate text color class.Using
text-error-11for descriptive footer text is semantically incorrect since this isn't an error message.- <div className="text-error-11 text-xs"> + <div className="text-gray-11 text-xs"> This is an example of a footer with a button for actions needed to be done </div>
12-15: Consider adding input validation to the example.The submit handler could demonstrate basic validation patterns for a more complete example.
const handleSubmit = () => { + if (!inputValue.trim()) { + return; // Could show validation message + } setInputResult(inputValue); setIsOpen(false); };internal/ui/src/components/dialog/dialog-container.tsx (1)
38-42: Consider extracting complex responsive classes for better maintainability.The responsive className configuration is quite complex and spans multiple lines. Consider extracting this into a separate constant or utility function to improve readability and maintainability.
+const dialogContentClasses = [ + "drop-shadow-2xl border-gray-4 max-h-[90vh] overflow-y-auto !rounded-2xl p-0 gap-0", + "w-[90%] md:w-[70%] lg:w-[70%] xl:w-[50%] 2xl:w-[45%] max-w-[600px]", + "max-h-[90vh] sm:max-h-[90vh] md:max-h-[70vh] lg:max-h-[90vh] xl:max-h-[80vh]" +].join(" "); <DialogContent className={cn( - "drop-shadow-2xl border-gray-4 max-h-[90vh] overflow-y-auto !rounded-2xl p-0 gap-0", - "w-[90%] md:w-[70%] lg:w-[70%] xl:w-[50%] 2xl:w-[45%] max-w-[600px] max-h-[90vh] sm:max-h-[90vh] md:max-h-[70vh] lg:max-h-[90vh] xl:max-h-[80vh]", + dialogContentClasses, className, )}apps/engineering/content/design/components/navigable-dialog.mdx (2)
9-9: Consider using standard spelling for "multi-step".The static analysis tool suggests "multistep" as the standard spelling.
-- Multi-step navigation with sidebar +- Multistep navigation with sidebar🧰 Tools
🪛 LanguageTool
[misspelling] ~9-~9: This word is normally spelled as one.
Context: ...igable-dialog.example"; ## Features - Multi-step navigation with sidebar - Icon support ...(EN_COMPOUNDS_MULTI_STEP)
87-87: Consider using standard spelling for "auto-focus".The static analysis tool suggests "autofocus" as the standard spelling.
-| preventAutoFocus | boolean | true | Whether to prevent auto-focus on open | +| preventAutoFocus | boolean | true | Whether to prevent autofocus on open |🧰 Tools
🪛 LanguageTool
[misspelling] ~87-~87: This word is normally spelled as one.
Context: ... | true | Whether to prevent auto-focus on open | | children | R...(EN_COMPOUNDS_AUTO_FOCUS)
apps/engineering/content/design/components/dialog-container.mdx (1)
67-67: Consider using standard spelling for "auto-focus".The static analysis tool suggests "autofocus" as the standard spelling.
-| preventAutoFocus | boolean | true | Whether to prevent auto-focus on open | +| preventAutoFocus | boolean | true | Whether to prevent autofocus on open |🧰 Tools
🪛 LanguageTool
[misspelling] ~67-~67: This word is normally spelled as one.
Context: ... | true | Whether to prevent auto-focus on open | | children ...(EN_COMPOUNDS_AUTO_FOCUS)
internal/ui/src/components/dialog/parts/dialog.tsx (2)
55-56: Consider splitting long class names for readabilityThe button class names string is quite long. Consider splitting it across multiple lines for better readability.
- const buttonClassNames = - "absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent text-muted-foreground z-[51]"; + const buttonClassNames = cn( + "absolute right-4 top-4 rounded-sm opacity-70", + "ring-offset-background transition-opacity hover:opacity-100", + "focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2", + "disabled:pointer-events-none data-[state=open]:bg-accent", + "text-muted-foreground z-[51]" + );
76-84: Address the TODO for more robust pointer down checksThe comment indicates that more robust checks might be needed for the pointer down outside handler. This could lead to edge cases where the dialog closes unexpectedly.
Would you like me to help implement a more robust solution or create an issue to track this improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/dashboard/app/(app)/apis/[apiId]/_components/create-key/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/_components/create-api-button.tsx(1 hunks)apps/engineering/content/design/components/dialog-container.mdx(1 hunks)apps/engineering/content/design/components/dialog/dialog-container.example.tsx(1 hunks)apps/engineering/content/design/components/dialog/navigable-dialog.example.tsx(1 hunks)apps/engineering/content/design/components/navigable-dialog.mdx(1 hunks)internal/ui/package.json(1 hunks)internal/ui/src/components/dialog/dialog-container.tsx(1 hunks)internal/ui/src/components/dialog/navigable-dialog.tsx(1 hunks)internal/ui/src/components/dialog/parts/dialog-parts.tsx(1 hunks)internal/ui/src/components/dialog/parts/dialog.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)packages/api/src/openapi.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/ui/src/components/dialog/parts/dialog.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
🪛 LanguageTool
apps/engineering/content/design/components/dialog-container.mdx
[misspelling] ~67-~67: This word is normally spelled as one.
Context: ... | true | Whether to prevent auto-focus on open | | children ...
(EN_COMPOUNDS_AUTO_FOCUS)
apps/engineering/content/design/components/navigable-dialog.mdx
[misspelling] ~9-~9: This word is normally spelled as one.
Context: ...igable-dialog.example"; ## Features - Multi-step navigation with sidebar - Icon support ...
(EN_COMPOUNDS_MULTI_STEP)
[misspelling] ~87-~87: This word is normally spelled as one.
Context: ... | true | Whether to prevent auto-focus on open | | children | R...
(EN_COMPOUNDS_AUTO_FOCUS)
🔇 Additional comments (14)
packages/api/src/openapi.d.ts (2)
3662-3662: LGTM! Improved example demonstrates human-readable permission names.The change from a slug-like example to a descriptive name better illustrates the intended usage of the
namefield for human-readable permission descriptions.
3665-3669: Excellent addition of slug field for unique identifiers.The new optional
slugfield provides a clean separation of concerns:
namefor human-readable descriptionsslugfor unique technical identifiers- Defaults to
nameif not provided, ensuring backward compatibilityThe API design improvement maintains existing functionality while providing better clarity.
internal/ui/package.json (1)
23-23: LGTM - Appropriate dependency for dialog components.The addition of
@radix-ui/react-dialogproperly supports the new dialog components being introduced in this package.apps/dashboard/app/(app)/apis/_components/create-api-button.tsx (1)
19-19:Details
✅ Verification successful
Clean migration to shared UI package.
The import path change from local component to
@unkey/uimaintains the dynamic loading pattern and aligns with the goal of consolidating dialog components.Let's verify that the DialogContainer export is available in the @unkey/ui package:
🏁 Script executed:
#!/bin/bash # Verify DialogContainer is exported from @unkey/ui package rg -A 3 "DialogContainer" internal/ui/src/index.tsLength of output: 50
Let's search the entire UI package for any definition or export of
DialogContainer:
🏁 Script executed:
#!/bin/bash # Comprehensive search for DialogContainer in UI package rg -n "DialogContainer" -A 3 internal/ui rg -n "export.*DialogContainer" -A 3 internal/ui rg -n "dialog-container" -A 3 internal/uiLength of output: 1740
DialogContainer export confirmed in shared UI package.
–
internal/ui/src/components/dialog/dialog-container.tsxdefines and exportsDialogContainer.
–internal/ui/src/index.tsre-exports it, making it available at@unkey/ui.The dynamic import change to
@unkey/uiis correct—no further action needed.internal/ui/src/index.ts (1)
4-5: Properly exposes new dialog components.The export statements correctly make the dialog components available as part of the
@unkey/uipackage's public API, enabling the migration seen in the dashboard files.internal/ui/src/components/dialog/dialog-container.tsx (2)
24-55: Well-structured component with proper TypeScript patterns.The DialogContainer implementation follows React best practices with:
- Proper TypeScript prop definitions
- Conditional rendering for optional elements
- Correct event handling for preventAutoFocus
- Clean composition of dialog parts
57-57: Clean re-export pattern for component composition.The re-export of dialog subcomponents provides a clean API for consumers who need more granular control over dialog composition.
apps/engineering/content/design/components/dialog/navigable-dialog.example.tsx (3)
18-26: Excellent performance optimization with memoization.The memoization of
TabContentand assignment ofdisplayNamefollows React best practices for performance and debugging.
29-70: Well-designed constant definitions prevent unnecessary re-renders.Pre-defining
NAV_ITEMSandCONTENT_ITEMSoutside the component prevents object recreation on each render, which is an excellent performance optimization.
73-99: Clean component composition and state management.The example demonstrates proper usage of the NavigableDialog components with straightforward state management and clear component composition.
apps/engineering/content/design/components/navigable-dialog.mdx (1)
1-122: Comprehensive and well-structured documentation.The documentation provides excellent coverage of the NavigableDialog component with:
- Clear feature descriptions
- Detailed component structure explanation
- Comprehensive prop tables
- Accessibility considerations
- Practical usage examples
🧰 Tools
🪛 LanguageTool
[misspelling] ~9-~9: This word is normally spelled as one.
Context: ...igable-dialog.example"; ## Features - Multi-step navigation with sidebar - Icon support ...(EN_COMPOUNDS_MULTI_STEP)
[misspelling] ~87-~87: This word is normally spelled as one.
Context: ... | true | Whether to prevent auto-focus on open | | children | R...(EN_COMPOUNDS_AUTO_FOCUS)
apps/engineering/content/design/components/dialog-container.mdx (1)
1-78: Well-crafted documentation with comprehensive coverage.The DialogContainer documentation effectively covers:
- Clear feature explanations
- Component structure breakdown
- Detailed prop specifications
- Accessibility considerations
- Practical usage examples
This provides excellent developer experience for using the component.
🧰 Tools
🪛 LanguageTool
[misspelling] ~67-~67: This word is normally spelled as one.
Context: ... | true | Whether to prevent auto-focus on open | | children ...(EN_COMPOUNDS_AUTO_FOCUS)
internal/ui/src/components/dialog/parts/dialog-parts.tsx (1)
18-29: Well-structured header componentGood implementation with proper TypeScript types and conditional rendering for the subtitle.
internal/ui/src/components/dialog/navigable-dialog.tsx (1)
234-234:Details
✅ Verification successful
Verify the scrollbar-hide utility class
The
scrollbar-hideclass might not be a standard Tailwind CSS utility. Ensure this custom class is defined in your CSS or consider using standard Tailwind utilities.If not found, you might need to add it to your global CSS or use Tailwind's scrollbar utilities if available.
🏁 Script executed:
#!/bin/bash # Check if scrollbar-hide is defined in CSS files rg -A 2 "scrollbar-hide" --type cssLength of output: 424
Confirmed: custom
scrollbar-hideutility is defined
Thescrollbar-hideclass is defined inapps/dashboard/styles/tailwind/tailwind.css, so no changes are needed here.
apps/engineering/content/design/components/dialog/dialog-container.example.tsx
Show resolved
Hide resolved
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Improvements
API Updates