feat: Container component #3257
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis change standardizes layout and container components across several dashboard and UI files by replacing custom or locally defined wrappers with new Changes
Sequence Diagram(s)sequenceDiagram
participant PageComponent
participant @unkey/ui
participant ChildComponent
PageComponent->>@unkey/ui: Render FlexibleContainer / ControlsContainer
@unkey/ui->>ChildComponent: Render children inside layout container
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (25)
✨ 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
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
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: 1
🔭 Outside diff range comments (1)
apps/dashboard/app/(app)/apis/[apiId]/page.tsx (1)
14-24:⚠️ Potential issuePreserve full-screen height styling
The original<div className="min-h-screen">enforced full-screen height. The new<FlexibleContainer>lacks this, which may alter layout. Re-add the class or equivalent prop.- <FlexibleContainer> + <FlexibleContainer className="min-h-screen">
♻️ Duplicate comments (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/controls/index.tsx (1)
1-1: Import updated to shared UI package
This mirrors the change in other control components. Ensure default styling consistency as noted above.apps/dashboard/app/(app)/ratelimits/_components/controls/index.tsx (1)
1-1: Import updated to shared UI package
Consistent with other control container migrations. Verify thatLogsSearchcontinues to accept thesetNamespacesandinitialNamespacesprops after this change.
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (1)
33-34: Use component props for alignment and padding
Instead of embeddingjustify-startanditems-startinclassName, leverage thejustify="start"andalign="start"props for clarity and consistency.- <FlexibleContainer padding="none" className="m-0 p-0 justify-start items-start"> + <FlexibleContainer padding="none" justify="start" align="start" className="m-0 p-0">apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx (1)
24-37: Consider specifying explicit props for FlexibleContainer components.While the replacement of divs with FlexibleContainer is good for standardization, consider explicitly specifying props like padding, align, and justify to ensure consistent behavior across different usages of FlexibleContainer. In the api-list-client.tsx file, padding="none" was explicitly set, but no props are specified here.
internal/ui/src/components/container/controls-container.tsx (1)
16-22: Clean implementation of ControlsLeft and ControlsRight components.Both components provide semantic structure for organizing controls on either side of the container with consistent styling.
Consider adding optional className props to these components for additional customization flexibility, similar to ControlsContainer:
-export function ControlsLeft({ children }: { children: React.ReactNode }) { - return <div className="flex gap-2">{children}</div>; +export function ControlsLeft({ + children, + className +}: { + children: React.ReactNode; + className?: string +}) { + return <div className={cn("flex gap-2", className)}>{children}</div>; } -export function ControlsRight({ children }: { children: React.ReactNode }) { - return <div className="flex gap-2">{children}</div>; +export function ControlsRight({ + children, + className +}: { + children: React.ReactNode; + className?: string +}) { + return <div className={cn("flex gap-2", className)}>{children}</div>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx(2 hunks)apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx(3 hunks)apps/dashboard/app/(app)/apis/[apiId]/page.tsx(3 hunks)apps/dashboard/app/(app)/apis/_components/api-list-client.tsx(3 hunks)apps/dashboard/app/(app)/apis/_components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/_components/controls/index.tsx(1 hunks)apps/dashboard/components/logs/controls-container.tsx(0 hunks)apps/engineering/content/design/components/container/flexible-container.example.tsx(1 hunks)apps/engineering/content/design/components/flexible-container.mdx(1 hunks)internal/ui/src/components/container/controls-container.tsx(1 hunks)internal/ui/src/components/container/flexible-container.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/components/logs/controls-container.tsx
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (1)
internal/ui/src/components/container/flexible-container.tsx (1)
FlexibleContainer(37-49)
apps/dashboard/app/(app)/apis/[apiId]/page.tsx (1)
internal/ui/src/components/container/flexible-container.tsx (1)
FlexibleContainer(37-49)
internal/ui/src/components/container/flexible-container.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx (1)
internal/ui/src/components/container/flexible-container.tsx (1)
FlexibleContainer(37-49)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (25)
internal/ui/src/components/container/flexible-container.tsx (1)
1-52: Well-designed FlexibleContainer component with appropriate variantsThis new component provides a flexible container with customizable padding, alignment, and justification options. It's a good abstraction that will help standardize layout patterns across the dashboard.
A few observations:
- Good use of
class-variance-authorityfor managing Tailwind CSS classes with variants- Appropriate defaults (centered alignment and justification with no padding)
- Well-typed props that extend HTML div attributes
The component design aligns well with the PR objectives of standardizing container components.
apps/dashboard/app/(app)/audit/components/controls/index.tsx (1)
2-2: Import update looks goodThe change to import control container components from
@unkey/uiinstead of a local path centralizes these components appropriately, improving maintainability without changing functionality.apps/dashboard/app/(app)/apis/_components/controls/index.tsx (1)
2-2: Import update looks goodThe change to import control container components from
@unkey/uiinstead of a local path centralizes these components appropriately, improving maintainability without changing functionality.apps/dashboard/app/(app)/logs/components/controls/index.tsx (1)
2-2: Import update looks goodThe change to import control container components from
@unkey/uiinstead of a local path centralizes these components appropriately, improving maintainability without changing functionality.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/index.tsx (2)
2-2:✅ Verification successful
Centralize control container imports
Good update replacing local control container components with the shared@unkey/uipackage. Ensure that@unkey/uiis added to your project’s dependencies.
🏁 Script executed:
#!/bin/bash # Verify that @unkey/ui is declared in package.json rg '"@unkey/ui"' -n package.jsonLength of output: 34
🏁 Script executed:
#!/bin/bash # Search all package.json files for @unkey/ui rg '"@unkey/ui"' -n -g 'package.json'Length of output: 215
@unkey/ui dependency confirmed
Great work centralizing the control container imports. I’ve verified that@unkey/uiis declared inapps/dashboard/package.json. Code changes approved.
12-27:✅ Verification successful
Validate default ControlsContainer styling
The newControlsContainermay have different default padding or alignment compared to the previous local implementation. Please confirm it matches the existing layout or explicitly pass props (e.g.,padding="none",justify="between") if needed.
🏁 Script executed:
#!/bin/bash # Inspect default props for ControlsContainer in UI package rg -A3 'export const ControlsContainer' internal/ui/src/components/container/controls-container.tsxLength of output: 101
🏁 Script executed:
#!/bin/bash set -e # Locate the ControlsContainer component file(s) echo "Searching for controls-container.tsx..." file_paths=$(fd controls-container.tsx) if [ -z "$file_paths" ]; then echo "No controls-container.tsx file found in the repo." exit 1 fi echo "Found:" echo "$file_paths" # Print the first 200 lines of each to inspect default props for path in $file_paths; do echo -e "\n=== $path (first 200 lines) ===" sed -n '1,200p' "$path" doneLength of output: 1591
No changes needed: ControlsContainer defaults match existing layout
Verified thatControlsContainerininternal/ui/src/components/container/controls-container.tsxalready usespadding="none"on itsFlexibleContainerand appliesjustify-betweenin the inner div (className="… justify-between …"), along withpx-3 py-2for spacing. No additional props are required.apps/dashboard/app/(app)/apis/[apiId]/api-id-navbar.tsx (1)
7-7:✅ Verification successful
Centralize layout container import
Updated to importFlexibleContainerfrom@unkey/ui. Ensure the package dependency is declared.
🏁 Script executed:
#!/bin/bash # Verify that @unkey/ui is declared in package.json rg '"@unkey/ui"' -n package.jsonLength of output: 34
🏁 Script executed:
#!/bin/bash # Search all package.json files for @unkey/ui declaration echo "Scanning package.json files for @unkey/ui..." find . -type f -name "package.json" -print | while read -r file; do echo ">> $file" grep -n '"@unkey/ui"' "$file" || echo " — not found" doneLength of output: 7046
Dependency declaration confirmed
Verified that@unkey/uiis declared inapps/dashboard/package.json(line 59). No further action needed.apps/dashboard/app/(app)/apis/[apiId]/page.tsx (1)
4-4:✅ Verification successful
Centralize layout container import
ImportedFlexibleContainerfrom@unkey/ui. Confirm the dependency is present.
🏁 Script executed:
#!/bin/bash # Verify that @unkey/ui is declared in package.json rg '"@unkey/ui"' -n package.jsonLength of output: 34
🏁 Script executed:
#!/bin/bash # Search all package.json files for @unkey/ui echo "🔍 Searching all package.json files for @unkey/ui..." find . -type f -name 'package.json' -print -exec rg '"@unkey/ui"' -n {} + # Check pnpm lockfile if present if [ -f pnpm-lock.yaml ]; then echo "🔍 Checking pnpm-lock.yaml for unkey/ui..." rg 'unkey/ui' -n pnpm-lock.yaml || echo "No entries found in pnpm-lock.yaml" fi # Check yarn lockfile if present if [ -f yarn.lock ]; then echo "🔍 Checking yarn.lock for unkey/ui..." rg 'unkey/ui' -n yarn.lock || echo "No entries found in yarn.lock" fiLength of output: 1724
Verified
@unkey/uiDependency
Confirmed that@unkey/uiis declared inapps/dashboard/package.json(line 59) and present in the lockfile. The import ofFlexibleContaineris valid.internal/ui/src/index.ts (1)
14-15: LGTM - Clean export additions for the new container components.The new export statements follow the established pattern and make the container components available throughout the application, which aligns with the PR objective of standardizing container components across the dashboard.
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx (2)
9-9: LGTM - Proper import of the new FlexibleContainer component.The import statement correctly adds the FlexibleContainer component from @unkey/ui alongside existing imports.
25-59: LGTM - Good replacement of custom div with standardized FlexibleContainer.The div with flex styling has been appropriately replaced with the standardized FlexibleContainer component. The padding="none" prop ensures the layout maintains the same spacing as before.
apps/dashboard/app/(app)/apis/[apiId]/_overview/logs-client.tsx (1)
4-4: LGTM - FlexibleContainer import added correctly.The import statement correctly adds the FlexibleContainer component from @unkey/ui.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/index.tsx (1)
1-1: LGTM - Proper imports for the controls container components.The import statement correctly imports the ControlsContainer, ControlsLeft, and ControlsRight components from @unkey/ui.
apps/engineering/content/design/components/flexible-container.mdx (6)
1-11: Well-structured documentation frontmatter and imports.The frontmatter and imports section provides clear information about the component and correctly imports all necessary example components.
Consider adding a simple installation and import instruction section to help developers quickly integrate this component:
## Installation and Import ```jsx import { FlexibleContainer } from "@unkey/ui";--- `13-16`: **Clear overview of the component's purpose.** The overview effectively communicates the component's purpose and key features. Consider enhancing this section with a basic code example to illustrate typical usage: ```markdown ```jsx <FlexibleContainer padding="medium" align="center" justify="center"> <YourContent /> </FlexibleContainer>--- `17-48`: **Excellent examples section with progression from simple to complex.** The examples section effectively demonstrates the various padding options and alignment settings available in the component. The progression from default to custom configurations provides a clear understanding of the component's capabilities. --- `49-57`: **Comprehensive props documentation.** The props table is well-formatted and includes all necessary information about the component's props, including types, default values, and descriptions. --- `58-64`: **Helpful best practices section.** The best practices section provides valuable guidance for using the component effectively. Consider adding a concrete example for combining with other layout components, as mentioned in the third bullet point: ```markdown For example, combining with Card component: ```jsx <FlexibleContainer padding="medium"> <Card> <CardContent>Your content here</CardContent> </Card> </FlexibleContainer>--- `65-67`: **Good accessibility note, but could be expanded.** The accessibility section acknowledges the importance of proper spacing and alignment for users with disabilities. Consider expanding this section with more specific guidance: ```markdown Consider the following accessibility aspects when using this component: - Maintain sufficient spacing between interactive elements (at least 8px) to help users with motor control difficulties - Ensure text within containers has sufficient color contrast with backgrounds - Use consistent alignment patterns throughout your application to help users with cognitive disabilitiesapps/engineering/content/design/components/container/flexible-container.example.tsx (4)
1-5: Proper setup with client directive and imports.The file correctly includes the "use client" directive and imports all necessary components from appropriate paths.
6-38: Well-structured default and small padding examples.Both DefaultExample and SmallExample components follow consistent patterns and effectively demonstrate their respective padding options. The examples use appropriate background colors and contain enough content to clearly illustrate the container's behavior.
40-72: Consistent medium and large padding examples.The MediumExample and LargeExample components maintain the established pattern and clearly demonstrate the "medium" and "large" padding options. The consistency across all examples makes the differences in padding sizes easy to understand.
74-115: Comprehensive custom alignment example with multiple configurations.The CustomExample component effectively demonstrates various alignment and justification combinations, showcasing the flexibility of the component. The layout is well-organized with appropriate flex containers and spacing.
The comment on line 104 about the height workaround could be more specific to help other developers understand the issue:
- {/* Needed to get height to work properly in RenderComponentWithSnippet */} + {/* These elements act as spacers to ensure consistent height rendering within + RenderComponentWithSnippet, which otherwise collapses without minimum content */}internal/ui/src/components/container/controls-container.tsx (2)
1-4: Clear comment and appropriate imports.The biome-ignore comment clearly explains why React is imported even though no React APIs are used directly. All necessary dependencies are properly imported.
5-14: Well-implemented ControlsContainer component.The ControlsContainer component effectively wraps children in a FlexibleContainer with appropriate styling. It properly handles the optional className prop using the cn utility function to merge with default styles.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/index.tsx
Outdated
Show resolved
Hide resolved
Total height of the controller container should be |
|
My thought process : 1.Make a simple container now to be used around the site. |
|
|
So when I need a div with flex I use this instead? |
|
I'm gonna ask a probably weird question. Do we have any style guides published? This kind of thing feels like it would go well in a style guide |
|
Don't think we have one |
|
Let’s brainstorm in iceland |
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?
Test A
Locations In Dashboard
Test B
Locations In Dashboard:
-/apis/[apiId]/api-id-navbar.tsx
-/apis/[apiId]/page.tsx
-/apis/[apiId]/_overview/logs-client.tsx
-/apis/_components/api-list-client.tsx
Test C Eng docs are sufficient.
Note: More options will be added to this component when the need arises.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor
Documentation