feat: design system #2667
feat: design system #2667
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several new React components and modifies existing ones across various files. Key additions include Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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! 🙏 |
chronark
requested review from
perkinsjr,
mcstepp and
MichaelUnkey
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: 13
🧹 Outside diff range and nitpick comments (29)
apps/engineering/postcss.config.js (1)
3-4: Consider pinning plugin versions for better stability.While the configuration is correct, consider pinning the plugin versions in package.json to prevent unexpected breaks from future updates.
Example package.json dependencies:
{ "devDependencies": { "postcss-import": "^15.1.0", "tailwindcss": "^3.0.0" } }internal/ui/src/lib/utils.ts (1)
4-6: Add JSDoc documentation for better developer experience.The implementation is solid and follows best practices for Tailwind projects. However, as this is a core utility for the design system, it would benefit from comprehensive documentation.
Consider adding JSDoc documentation:
+/** + * Combines multiple class names into a single string, resolving Tailwind CSS conflicts. + * @param inputs - Array of class names, conditional classes, or class name objects + * @returns Merged class names string with resolved Tailwind conflicts + * @example + * ```tsx + * // Basic usage + * cn('px-2 py-1', 'bg-blue-500') + * // With conditions + * cn('px-2', isActive && 'bg-blue-500') + * // With Tailwind conflicts + * cn('px-2 py-1', 'px-4') // px-4 wins + * ``` + */ export function cn(...inputs: ClassValue[]) { return twMerge(clsx(inputs)); }apps/engineering/app/components/row.tsx (2)
3-5: Consider enhancing component configurability.The component could be more reusable by accepting props for customizing spacing and alignment. This would make it more flexible for different use cases within the design system.
Consider this enhancement:
-export const Row: React.FC<PropsWithChildren> = (props) => { +interface RowProps extends PropsWithChildren { + gap?: 'sm' | 'md' | 'lg'; // maps to gap-4, gap-8, gap-12 + justify?: 'start' | 'center' | 'end' | 'around' | 'between'; + align?: 'start' | 'center' | 'end'; +} + +export const Row: React.FC<RowProps> = ({ + children, + gap = 'md', + justify = 'around', + align = 'center' +}) => { + const gapMap = { + sm: 'gap-4', + md: 'gap-8', + lg: 'gap-12' + }; + return ( - <div className="flex w-full items-center justify-around gap-8"> - {props.children} + <div + className={` + flex w-full + items-${align} + justify-${justify} + ${gapMap[gap]} + `} + > + {children} </div> ); };
3-5: Consider adding semantic HTML attributes.For better accessibility, consider adding appropriate ARIA attributes or semantic HTML roles when necessary, depending on the content being wrapped.
Example enhancement:
- return <div className="flex w-full items-center justify-around gap-8">{props.children}</div>; + return ( + <div + role="group" + aria-label="Content Row" + className="flex w-full items-center justify-around gap-8" + > + {props.children} + </div> + );apps/engineering/next.config.mjs (1)
8-8: Consider adding a comment explaining the transpilation requirement.It would be helpful to document why @unkey/ui needs transpilation, especially for future maintainers.
+ // Transpile @unkey/ui to ensure React components are properly processed by Next.js transpilePackages: ["@unkey/ui"],apps/engineering/app/design/layout.tsx (2)
8-8: Consider documenting spread props for maintainabilityThe spreading of
baseOptionsmakes it less obvious which props are being passed toDocsLayout. Consider adding a comment documenting the key properties frombaseOptionsor destructuring them explicitly.+ // baseOptions includes: {editLink, search, navigation} props for DocsLayout <DocsLayout tree={componentSource.pageTree} {...baseOptions}>
1-12: Consider enhancing documentation structureSince this is a design system documentation layout, consider these architectural improvements:
- Add theme switching capability for previewing components in different themes
- Consider implementing a component playground section
- Add versioning support for the design system documentation
These features would make the documentation more robust and user-friendly.
Would you like me to provide implementation examples for any of these suggestions?
apps/engineering/app/source.ts (1)
Line range hint
1-18: Consider unifying documentation source configurationWith the addition of
componentSource, we now have three separate documentation sources (source,rfcSource, andcomponentSource), each with its own configuration. Consider creating a unified configuration system that could:
- Centralize the documentation source definitions
- Share common configuration
- Make it easier to add new documentation types
Example approach:
const docSources = { docs: { baseUrl: '/docs', meta: meta, }, rfcs: { baseUrl: '/rfcs', meta: [], }, components: { baseUrl: '/design', meta: [], }, }; export const createSource = (key: keyof typeof docSources) => loader({ baseUrl: docSources[key].baseUrl, source: createMDXSource(sources[key], docSources[key].meta), });apps/engineering/app/layout.tsx (1)
11-15: Consider using array join for better className readabilityWhile the current implementation works, consider using array join for better maintainability:
- className={`${GeistSans.variable} ${GeistMono.variable}`} + className={[GeistSans.variable, GeistMono.variable].join(' ')}apps/engineering/source.config.ts (1)
19-23: Consider enhancing the schema with useful metadata.While the basic setup looks good, consider extending the schema with metadata fields that could be valuable for design system documentation, such as:
- Component status (draft, beta, stable)
- Last reviewed date
- Design principles/guidelines references
- Accessibility considerations
Example schema extension:
export const components = defineCollections({ dir: "content/design", - schema: frontmatterSchema.extend({}), + schema: frontmatterSchema.extend({ + status: z.enum(["draft", "beta", "stable"]).default("draft"), + lastReviewed: z.string().date().or(z.date()).optional(), + designPrinciples: z.array(z.string()).optional(), + a11y: z.string().optional(), + }), type: "doc", });apps/engineering/app/components/render.tsx (1)
11-14: Consider adding JSX conversion options as props.The JSX string conversion options are currently hardcoded. Consider making them configurable through props for better reusability.
+interface RenderComponentWithSnippetProps extends PropsWithChildren { + showFunctions?: boolean; + useBooleanShorthand?: boolean; +} -export const RenderComponentWithSnippet: React.FC<PropsWithChildren> = (props) => { +export const RenderComponentWithSnippet: React.FC<RenderComponentWithSnippetProps> = ({ + children, + showFunctions = true, + useBooleanShorthand = true, +}) => {internal/ui/tailwind.config.js (1)
24-25: Improve documentation for the button shadowThe comment "from vitor's figma" could be more descriptive about the design decision.
Consider updating the comment to be more informative:
- // from vitor's figma + // Design spec: Standard button shadow for depth and interaction feedbackapps/engineering/app/design/[[...slug]]/page.tsx (2)
11-17: Enhance error handling and loading statesThe current implementation only handles the "not found" case. Consider adding:
- Error boundaries for handling runtime errors
- Loading states for better UX
- Validation for the page data structure
Consider wrapping the component with error boundaries and adding loading states:
+import { ErrorBoundary } from 'react-error-boundary'; +import { Suspense } from 'react'; export default async function Page(props: { params: Promise<{ slug?: string[] }>; }) { + return ( + <ErrorBoundary fallback={<div>Something went wrong</div>}> + <Suspense fallback={<div>Loading...</div>}> + <PageContent params={props.params} /> + </Suspense> + </ErrorBoundary> + ); }
28-30: Add type safety to MDX componentsThe MDX components spread operation could benefit from explicit typing to ensure all required components are provided.
Consider adding type safety:
+import type { MDXComponents } from 'mdx/types' + <DocsBody> - <MDX components={{ ...defaultMdxComponents }} /> + <MDX components={{ ...defaultMdxComponents } satisfies MDXComponents} /> </DocsBody>apps/engineering/app/docs/[[...slug]]/page.tsx (1)
55-56: Consider simplifying metadata parameter handlingThe Promise wrapper around params adds an extra layer of complexity. If possible, consider keeping the original synchronous parameter type:
-export async function generateMetadata({ params }: { params: Promise<{ slug?: string[] }> }) { - const page = source.getPage((await params).slug); +export async function generateMetadata({ params }: { params: { slug?: string[] } }) { + const page = source.getPage(params.slug);If the Promise-based params are part of a larger architectural decision, consider documenting this pattern and its rationale in the codebase to help other developers understand the design choice.
apps/engineering/app/rfcs/[[...slug]]/page.tsx (4)
10-14: Consider using PageProps type from Next.js.The async params handling is correct, but for better type safety and consistency with Next.js conventions, consider using the built-in PageProps type.
-export default async function Page(props: { - params: Promise<{ slug?: string[] }>; -}) { +import { PageProps } from 'next'; + +export default async function Page({ + params, +}: PageProps<{ slug?: string[] }>) { - const params = await props.params;
63-64: Consider using GenerateMetadata type from Next.js.The async params handling is correct, but for better type safety and consistency with Next.js conventions, consider using the built-in GenerateMetadata type.
-export async function generateMetadata({ params }: { params: Promise<{ slug?: string[] }> }) { +import { Metadata, ResolvingMetadata } from 'next'; + +export async function generateMetadata( + { params }: PageProps<{ slug?: string[] }>, + parent: ResolvingMetadata +): Promise<Metadata> {
Line range hint
26-33: Consider improving the empty state layout.The current implementation uses hardcoded spacing (-mt-16) and multiple utility classes. Consider extracting this into a reusable component with more maintainable styling.
- <div className="min-h-screen border text-center -mt-16 pt-16 flex items-center w-screen justify-center "> + <div className="min-h-[calc(100vh-4rem)] border text-center flex items-center justify-center"> <div> - <h1 className="text-7xl md:text-8xl font-bold leading-none uppercase tracking-tight"> + <h1 className="text-7xl md:text-8xl font-bold leading-none uppercase tracking-tight"> RFCS </h1> - <p className="text-xl mt-8 font-light "> + <p className="text-xl mt-8 font-light">
Line range hint
43-49: Consider using a more semantic structure for RFC metadata.The current grid layout could be improved using a definition list (dl, dt, dd) for better semantics while maintaining the same visual appearance.
- <div className="grid grid-cols-2 font-mono"> - <span>ID</span> - <span>{page.data.title.split(" ").at(0)}</span> - <span>{page.data.authors.length > 1 ? "Authors" : "Author"}</span> - <span>{page.data.authors.join(", ")}</span> - <span>Date</span> - <LocalDate date={new Date(page.data.date)} /> - </div> + <dl className="grid grid-cols-2 font-mono gap-y-2"> + <dt>ID</dt> + <dd>{page.data.title.split(" ").at(0)}</dd> + <dt>{page.data.authors.length > 1 ? "Authors" : "Author"}</dt> + <dd>{page.data.authors.join(", ")}</dd> + <dt>Date</dt> + <dd><LocalDate date={new Date(page.data.date)} /></dd> + </dl>apps/engineering/content/design/components/button.mdx (2)
10-15: Consider enhancing the onClick exampleWhile the current example demonstrates the basic usage, consider showing more practical examples such as:
- Form submission
- State updates
- Async operations with loading states
- <Button onClick={()=>alert("hello")}>Trigger Alert</Button> + <Button onClick={() => console.log("Clicked!")}>Log Click</Button> + <Button + onClick={async () => { + // Example with loading state + setLoading(true); + await saveData(); + setLoading(false); + }} + > + Save Data + </Button>
35-43: Consider adding accessibility documentation for icon buttonsThe icons demonstration is good, but consider adding examples showing how to make icon-only buttons accessible (if supported by the component).
+ {/* Example with aria-label for accessibility */} + <Button prefix={<Settings2/>} aria-label="Settings"> + <span className="sr-only">Open settings</span> + </Button>apps/engineering/content/design/colors.mdx (3)
6-11: Remove unnecessary empty linesMultiple consecutive empty lines don't add value to the documentation. Consider keeping just one or two for spacing.
--- title: Colors description: Color scale and usage --- - - - - - Unkey uses [Radix Colors](https://www.radix-ui.com/colors) and its scale.
12-12: Enhance the introduction with more contextConsider adding more context about why Radix Colors was chosen and its benefits for the design system.
-Unkey uses [Radix Colors](https://www.radix-ui.com/colors) and its scale. +Unkey uses [Radix Colors](https://www.radix-ui.com/colors) for its comprehensive, accessible, and consistent color system. This scale provides semantic meaning across different states and ensures WCAG compliance for better accessibility.
14-28: Consider adding visual examples to the usage tableThe table provides clear documentation, but adding small visual examples for each use case would make it more practical and easier to understand.
Consider adding a third column with small visual examples or code snippets demonstrating each use case.
internal/ui/src/components/button.tsx (5)
8-14: Complete or remove incomplete comment block and fix typographical errors.The comment block between lines 8-14 contains incomplete thoughts and misspellings, such as "keyboard select stae" instead of "keyboard select state". Please complete the comments to provide clarity or remove them if they are no longer necessary to maintain code cleanliness.
46-46: Duplicateitems-centerclass inkeyboardIconVariants.The class names include
items-centertwice. Removing the duplicate avoids redundancy and potential confusion.Apply this diff to remove the duplicate class:
- "items-center transition duration-150 text-center items-center justify-center shadow-none size-5 flex justify-center font-mono text-xs font-medium border rounded-[5px]", + "items-center transition duration-150 text-center justify-center shadow-none size-5 flex justify-center font-mono text-xs font-medium border rounded-[5px]",
68-68: Redundantdisabledprop definition inButtonProps.The
disabledprop is already included inReact.ButtonHTMLAttributes<HTMLButtonElement>, so redefining it inButtonPropsis unnecessary.Apply this diff to remove the redundant definition:
loading?: boolean; - disabled?: boolean; /**
96-111: Potential performance issue due to multiplekeydownevent listeners ondocument.Each instance of the
Buttoncomponent with akeyboardprop adds akeydownevent listener to thedocument. If multiple buttons are rendered, this could lead to performance degradation and unexpected behavior due to multiple listeners. Consider centralizing the event listener or using a global context or a library to manage keyboard shortcuts more efficiently.
117-121: RedundantonClickprop assignment.The
onClickhandler is explicitly assigned viaonClick={props.onClick}and also included in{...props}. This duplication is unnecessary unless there's a specific reason for it.Apply this diff to remove the redundant assignment:
className={cn(buttonVariants({ variant, size, className }))} ref={ref} - onClick={props.onClick} {...props}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
apps/engineering/app/components/render.tsx(1 hunks)apps/engineering/app/components/row.tsx(1 hunks)apps/engineering/app/design/[[...slug]]/page.tsx(1 hunks)apps/engineering/app/design/layout.tsx(1 hunks)apps/engineering/app/docs/[[...slug]]/page.tsx(2 hunks)apps/engineering/app/layout.config.tsx(1 hunks)apps/engineering/app/layout.tsx(1 hunks)apps/engineering/app/rfcs/[[...slug]]/page.tsx(2 hunks)apps/engineering/app/source.ts(2 hunks)apps/engineering/content/design/colors.mdx(1 hunks)apps/engineering/content/design/components/button.mdx(1 hunks)apps/engineering/content/design/index.mdx(1 hunks)apps/engineering/content/design/meta.json(1 hunks)apps/engineering/content/docs/meta.json(1 hunks)apps/engineering/next.config.mjs(1 hunks)apps/engineering/package.json(1 hunks)apps/engineering/postcss.config.js(1 hunks)apps/engineering/source.config.ts(1 hunks)apps/engineering/tailwind.config.js(2 hunks)apps/play/package.json(1 hunks)internal/ui/colors.css(1 hunks)internal/ui/components.json(1 hunks)internal/ui/css.ts(1 hunks)internal/ui/package.json(1 hunks)internal/ui/src/components/button.tsx(1 hunks)internal/ui/src/index.ts(1 hunks)internal/ui/src/lib/utils.ts(1 hunks)internal/ui/tailwind.config.js(1 hunks)internal/ui/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- apps/engineering/content/design/meta.json
- apps/engineering/content/docs/meta.json
- apps/play/package.json
- internal/ui/colors.css
- internal/ui/components.json
- internal/ui/css.ts
- internal/ui/package.json
- internal/ui/src/index.ts
- internal/ui/tsconfig.json
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/design/index.mdx
[grammar] ~5-~5: The word ‘404’ seems to be an ordinal number with a missing ordinal indicator (‘st’, ‘nd’, ‘rd’ or ‘th’).
Context: ...t's needed for fumadocs to not render a 404 page. The content of this file are not ...
(ORDINAL_NUMBER_MISSING_ORDINAL_INDICATOR)
🔇 Additional comments (32)
apps/engineering/postcss.config.js (1)
3-4: LGTM! Plugin order is correct.
The addition of postcss-import and tailwindcss/nesting plugins is properly configured with the correct ordering. The postcss-import plugin is correctly placed first to process imports before other transformations, and tailwindcss/nesting is correctly placed before the main tailwindcss plugin.
Let's verify if these plugins are actually needed by the new components:
✅ Verification successful
The PostCSS plugins are actively used and properly configured
The verification confirms that both plugins are necessary:
postcss-importis actively used for CSS modularization, as evidenced by multiple@importstatements in the dashboard stylestailwindcss/nestingis required for the project as:- It's consistently configured across multiple PostCSS configs (found in both engineering and dashboard apps)
- Complex CSS nesting patterns are present in the typography styles
- The codebase uses Tailwind's
@layerdirectives with nested rules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for CSS imports and nesting usage in the codebase
echo "Checking for CSS @import usage..."
rg "@import" --type css
echo "Checking for nested CSS patterns..."
rg "{\s*&" --type css
Length of output: 569
Script:
#!/bin/bash
# Check for nested CSS patterns with a corrected regex
echo "Checking for nested CSS patterns..."
rg "{\s*&|&[^{]+{" --type css
# Check if tailwind/nesting is used in other postcss configs
echo -e "\nChecking for other postcss configs using tailwind/nesting..."
fd "postcss.config.js" --exec grep -l "tailwindcss/nesting" {}
# Look for CSS files that might use nesting
echo -e "\nChecking CSS files content..."
fd -e css --exec head -n 20 {}
Length of output: 4178
internal/ui/src/lib/utils.ts (2)
1-2: LGTM! Clean and well-structured imports.
The imports are properly organized and include both the necessary type and functions.
4-6: Verify consistent usage across the design system.
As this is a core utility for the design system, let's ensure it's being used consistently and there are no competing implementations.
✅ Verification successful
Based on the comprehensive search results, I can now generate the final response:
The cn utility is being used consistently across the codebase
The verification shows:
- The
cnutility is defined consistently in multiple locations using the same implementation - It's being used extensively throughout the codebase for class name merging
- No competing or duplicate class merging utilities were found
- No direct usage of
clsx()ortwMerge()was found outside thecnutility implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of cn utility and potential duplicate implementations
# Check for other class name merging utilities
echo "Checking for potential duplicate implementations..."
rg -g "*.{ts,tsx}" "function.*merge.*class|function.*combine.*class"
# Check current usage of cn function
echo "Checking current usage patterns..."
rg -g "*.{ts,tsx}" "cn\(" -A 2
# Look for direct usage of clsx or twMerge that could be replaced with cn
echo "Checking for direct library usage that could use cn instead..."
rg -g "*.{ts,tsx}" "clsx\(|twMerge\("
Length of output: 110256
apps/engineering/app/components/row.tsx (2)
1-1: LGTM! Clean and type-safe import.
The use of PropsWithChildren type from React is the recommended approach for typing components that accept children.
3-5: LGTM! Clean implementation of the Row component.
The component follows React best practices and implements a clear layout structure using Tailwind CSS.
apps/engineering/app/design/layout.tsx (2)
6-12: LGTM! Clean and well-structured layout component.
The implementation follows React best practices and properly handles the documentation layout structure.
1-5: Verify path alias configuration for "@/" imports
The @/ path alias is used for importing componentSource. Let's ensure the path alias is properly configured in the TypeScript and build configuration.
✅ Verification successful
Path alias @/ is properly configured and widely used
The @/ path alias is correctly configured and extensively used throughout the codebase, particularly in the Next.js applications. The import @/app/source in apps/engineering/app/design/layout.tsx follows the established pattern used across the project.
- The path alias is used consistently across multiple apps (www, dashboard, engineering, etc.)
- It's a standard Next.js convention for absolute imports from the app's root
- The pattern is used for importing components, utilities, styles, and other modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for path alias configuration in tsconfig
fd tsconfig.json | xargs cat | jq '.compilerOptions.paths'
# Check for path alias in build config (if using Next.js)
fd next.config. | xargs cat
Length of output: 3390
Script:
#!/bin/bash
# Let's check the tsconfig files more carefully without using jq
fd tsconfig.json --exec cat {}
# Also check if the @/ alias is used elsewhere in the codebase
rg "@/" --type ts --type tsx
Length of output: 100164
Script:
#!/bin/bash
# Let's check for Next.js config files in the apps/engineering directory
fd next.config. apps/engineering
# And check for tsconfig in the same directory
fd tsconfig.json apps/engineering
# Also check for any usage of @/ imports in the codebase with ripgrep
rg "@/" --type ts
Length of output: 101856
apps/engineering/app/source.ts (2)
1-1: LGTM! Clean import addition
The new components import follows the existing pattern and is properly grouped with related imports.
15-18: Verify meta configuration needs
The implementation follows the existing pattern consistently. However, while rfcSource uses an empty array for meta configuration, the main source uses the meta import.
Let's verify if any meta configuration exists for components:
✅ Verification successful
Let me gather more specific information about the meta configuration in the engineering app.
Let me check the meta import and its usage in the engineering app source file.
Empty meta configuration is appropriate for components
The implementation is correct as is. Looking at the source file, there are three distinct source configurations:
source- usesmetafor documentation pagesrfcSource- uses empty array[]for RFC pagescomponentSource- uses empty array[]for component pages
The empty meta configuration for components follows the same pattern as rfcSource, which is appropriate since both are specialized content types that don't require the same metadata as the main documentation pages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any meta configuration files for components
# that should be included in the createMDXSource call
# Look for potential meta configuration files
rg -g '!node_modules' -g '!.git' -l 'meta' $(fd -t f . --base-directory @/.source)
Length of output: 7124
Script:
#!/bin/bash
# Check for meta configuration in the engineering app source files
rg -g '!node_modules' -g '!.git' "meta.*=.*\[" apps/engineering/
rg -g '!node_modules' -g '!.git' "createMDXSource" apps/engineering/
Length of output: 426
Script:
#!/bin/bash
# Check the content of source.ts to understand meta usage
cat apps/engineering/app/source.ts
Length of output: 490
apps/engineering/tailwind.config.js (2)
1-1: LGTM: Design system integration via shared config
The import of unkeyUiTailwindConfig aligns with the PR's objective of implementing a unified design system.
11-11: Verify UI components path resolution
The new path "../../internal/ui/**/*.tsx" suggests UI components are being shared across apps.
Let's verify the path resolution and existence of UI components:
✅ Verification successful
Path resolution for UI components is correctly configured
The verification confirms that:
- The UI components directory exists and is accessible from the engineering app
- There are shared UI components present (e.g., button.tsx)
- The path resolution in tailwind.config.js is correctly configured to reach these components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of UI components and their location
fd -e tsx . internal/ui/ -x echo "Found UI component: {}"
# Check if the path resolves correctly from the engineering app
cd apps/engineering
if [ -d "../../internal/ui" ]; then
echo "UI directory is accessible from engineering app"
else
echo "Warning: UI directory not found at expected location"
fi
Length of output: 275
apps/engineering/app/layout.tsx (2)
2-3: LGTM: Good choice of font system!
The switch to Geist fonts is a solid choice for a design system. Having both Sans and Mono variants will provide good typographic hierarchy between content and code snippets.
13-13: Verify Tailwind configuration for Geist fonts
Let's ensure the font variables are properly configured in the Tailwind setup.
✅ Verification successful
Geist fonts are properly configured and used
The verification shows that:
- Geist fonts are correctly imported from the
geistpackage - Font variables are properly applied to the HTML root element
- The setup follows Next.js best practices for font configuration
The implementation is correct and doesn't require any changes to the Tailwind configuration since Geist font variables are handled through Next.js's built-in font system rather than Tailwind's font family configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Tailwind config for font family configuration
fd -e js -e ts -e cjs -e mjs tailwind.config && xargs cat {} \
| grep -A 10 "fontFamily"
# Verify if the font is being used in other components
rg "GeistSans.variable|GeistMono.variable" --type tsx --type jsx
Length of output: 445
Script:
#!/bin/bash
# Let's check the content of the tailwind config files we found
echo "=== apps/engineering/tailwind.config.js ==="
cat apps/engineering/tailwind.config.js
# Check if Geist fonts are imported in the layout file
echo -e "\n=== apps/engineering/app/layout.tsx ==="
cat apps/engineering/app/layout.tsx
# Check for any font configuration in the project
echo -e "\n=== Searching for font imports ==="
rg "from.*[@]geist" --type ts --type tsx --type jsx
Length of output: 1430
apps/engineering/app/components/render.tsx (2)
1-3: LGTM! Clean and well-organized imports.
The imports are properly structured with clear separation of component, type, and utility imports.
5-19: Verify integration with design system theme.
Since this component is part of a design system implementation, ensure it properly integrates with the theme configuration.
✅ Verification successful
The component correctly integrates with the design system theme
The component uses CodeBlock and Pre from fumadocs-ui/components/codeblock, which is a UI component library that handles theming internally. No additional theme integration is required as these components are pre-styled and maintain consistency with the design system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for theme configuration files and usage
echo "Checking for theme configuration..."
fd -e ts -e js -e json "theme|tailwind" -x cat {}
# Check for similar component implementations
echo "Checking for similar rendering components..."
rg -t tsx -t jsx "CodeBlock.*Pre"
Length of output: 17295
Script:
#!/bin/bash
# Check for theme-related imports and styling in the component's file
echo "Checking theme-related imports and styling in render.tsx..."
rg -A 5 -B 5 "import.*theme|className|style" "apps/engineering/app/components/render.tsx"
# Check CodeBlock and Pre component definitions
echo "Checking CodeBlock and Pre component definitions..."
fd -e tsx -e jsx "CodeBlock|Pre" -x cat {}
# Check for theme usage in similar components
echo "Checking theme usage in similar components..."
rg --type ts --type tsx --type jsx "CodeBlock.*className|Pre.*className"
Length of output: 582
Script:
#!/bin/bash
# Check for the CodeBlock and Pre components implementation
echo "Checking for CodeBlock and Pre components..."
rg -g "*.{tsx,jsx}" -l "CodeBlock|Pre"
# Check the specific file content
echo "Checking render.tsx content..."
cat "apps/engineering/app/components/render.tsx"
# Look for styled-components or CSS modules
echo "Checking for style files..."
fd -e css -e styled.tsx -e styles.tsx -p "apps/engineering/app/components"
Length of output: 2375
apps/engineering/app/layout.config.tsx (1)
38-42: LGTM! The new navigation link is well-structured.
The addition of the Design section link follows the established pattern and is consistently formatted with other internal navigation items.
Let's verify that the design route exists:
✅ Verification successful
The design route and content are properly set up
The verification confirms:
- The design route exists at
apps/engineering/app/design/[[...slug]]/page.tsx - Design content is available in
apps/engineering/content/design/with multiple MDX files - The
/designURL is correctly referenced in bothsource.tsandlayout.config.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of design route and related files
# Check for design-related files and routes
echo "Checking for design route and related files..."
fd -t f "page.tsx" apps/engineering/app/design
fd -t f . apps/engineering/content/design
# Check for any references to the design route
echo "Checking for route references..."
rg -g "*.tsx" -g "*.ts" -g "*.mdx" '"/design"'
Length of output: 661
apps/engineering/package.json (5)
20-20: LGTM: Geist font addition
The addition of geist font package aligns with the design system objectives.
23-24: Verify React version compatibility
React v18.3.1 is specified. Ensure this version is compatible with Next.js 15 and other dependencies.
13-13: Verify workspace dependency resolution
The @unkey/ui package is using a workspace dependency. Ensure it's properly linked and versioned across all apps using it.
✅ Verification successful
The first command will help us verify if other packages are using @unkey/ui, and the second command will confirm if the package exists in the workspace. This will help us determine if the workspace dependency is properly set up.
This will help us confirm if the internal/ui package is indeed the workspace package being referenced and verify its configuration.
Workspace dependency is correctly configured
The @unkey/ui workspace dependency is properly set up:
- Package exists as
internal/uiwith correct package name@unkey/ui - Only the engineering app is consuming it as a workspace dependency
- The UI package itself is properly configured with its own dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent workspace dependency usage
fd package.json | xargs rg "@unkey/ui.*workspace:" -A 1 -B 1
Length of output: 253
Script:
#!/bin/bash
# Let's check if @unkey/ui package exists in the workspace and its dependencies
fd "package.json" --type f --exec grep -l "@unkey/ui" {} \;
# Also check if there's a packages/ui directory that might contain the workspace package
fd --type d --max-depth 2 "^ui$" --base-directory packages
Length of output: 180
Script:
#!/bin/bash
# Let's check the package.json content of the internal/ui package to verify it's the workspace package
cat internal/ui/package.json
Length of output: 700
25-25: Verify usage of react-element-to-jsx-string
The addition of react-element-to-jsx-string suggests new documentation features. Ensure it's properly utilized and doesn't impact bundle size unnecessarily.
✅ Verification successful
Package is properly used for documentation purposes
The react-element-to-jsx-string package is used appropriately in a documentation-focused component (RenderComponentWithSnippet) that displays both a React component and its JSX representation. The implementation is clean and the package is only imported where needed, which is good for code splitting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check where the package is being used
rg "from ['|\"]react-element-to-jsx-string" --type=typescript
# Look for dynamic imports to verify if it's properly code-split
rg "dynamic|import\(.*react-element-to-jsx-string" --type=typescript
Length of output: 206
Script:
#!/bin/bash
# Check where the package is being used, searching all relevant file types
rg "from ['|\"]react-element-to-jsx-string" -t js -t jsx -t ts -t tsx
# Look for any other import patterns of this package
rg "require.*react-element-to-jsx-string|import.*react-element-to-jsx-string" -t js -t jsx -t ts -t tsx
# Check for actual usage of functions from this package
rg "reactElementToJsxString|elementToString" -t js -t jsx -t ts -t tsx
Length of output: 336
Script:
#!/bin/bash
# Search for any files that might use this package, without specifying file types
rg "react-element-to-jsx-string"
# Find all JavaScript/TypeScript files that might contain the usage
fd -e js -e ts -e jsx -e tsx
Length of output: 49138
Script:
#!/bin/bash
# Check the content of the file where we found the usage
cat apps/engineering/app/components/render.tsx
Length of output: 589
22-22: Verify Next.js v15 compatibility
The upgrade to Next.js v15.0.3 is a major version change that requires careful verification:
- Check the migration guide for breaking changes
- Ensure all Next.js related dependencies are compatible
internal/ui/tailwind.config.js (2)
32-49: Well-implemented color palette helper
The palette helper function is well-documented and efficiently implements a 12-step color scale using CSS variables. This approach provides good flexibility and maintainability for the color system.
6-9: Verify font variable availability
The font configuration looks good, but ensure these CSS variables are defined in your CSS setup.
✅ Verification successful
Font variables are properly configured and used
The Geist fonts are correctly set up in the project:
- The font variables are properly defined through the Geist font package's built-in
.variableproperty in multiple layout files (e.g.,apps/www/app/layout.tsx,apps/engineering/app/layout.tsx) - The CSS variables are consistently used across multiple tailwind configurations (
apps/www/tailwind.config.ts,internal/ui/tailwind.config.js) - The fonts are imported from
geist/font/monoandgeist/font/sansand applied via the appropriate className configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if font variables are defined in CSS files
rg --type css "font-geist-(sans|mono)"
Length of output: 40
Script:
#!/bin/bash
# Let's check for any CSS or related files that might define these font variables
fd '\.(css|scss|sass|less|module\.css)$' | xargs rg "font-geist|--font-geist"
# Also check for potential font loading/configuration in JavaScript/TypeScript files
fd '\.(js|jsx|ts|tsx)$' | xargs rg "font-geist|Geist"
Length of output: 1448
apps/engineering/app/design/[[...slug]]/page.tsx (1)
5-5: Consider completing the TypeScript integration
There are commented-out imports and usage of createTypeTable from fumadocs-typescript. If TypeScript documentation is planned, we should either:
- Complete the integration and uncomment these lines
- Remove the commented code if it's not needed
Let's check if TypeScript documentation is used elsewhere in the codebase:
Also applies to: 18-18
apps/engineering/app/docs/[[...slug]]/page.tsx (2)
Line range hint 1-64: Overall implementation is robust with good error handling
The documentation page implementation is well-structured with proper error handling, metadata generation, and last update tracking. The integration with fumadocs-ui components provides a consistent documentation experience.
8-11: Verify Next.js parameter handling approach
The change to handle params as a Promise seems unusual for Next.js page components. This might impact performance due to the additional async resolution step.
Let's verify if this is a common pattern in the codebase and check Next.js documentation:
apps/engineering/content/design/components/button.mdx (3)
1-8: LGTM!
The imports and metadata are well-structured and appropriate for the documentation.
21-30: LGTM!
The variants demonstration is comprehensive and well-organized, showing all available button styles with appropriate action labels.
97-106: LGTM!
The disabled states demonstration is comprehensive and well-implemented, showing all variants in their disabled state.
apps/engineering/content/design/colors.mdx (1)
1-142: Verify Tailwind color configuration
Let's ensure all color classes used in the documentation are properly configured in Tailwind.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have enough information to verify the color classes used in the documentation.
Color classes are properly configured in Tailwind
The color classes used in the documentation (gray, success, info, warning, error, feature) are correctly configured in the Tailwind configuration files. Specifically:
- The main UI configuration in
internal/ui/tailwind.config.jsdefines all the required color scales:
colors: {
gray: palette("gray"),
info: palette("info"),
success: palette("success"),
warning: palette("warning"),
error: palette("error"),
feature: palette("feature")
}Additionally, the project uses @radix-ui/colors package which provides the color scales, as confirmed by the package dependencies in internal/ui/package.json:
"@radix-ui/colors": "^3.0.0"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all color classes are configured in Tailwind
# Look for Tailwind config files
echo "Searching for Tailwind configuration..."
fd -e js -e ts -e cjs -e mjs -g "*tailwind*config*"
# Check for color definitions in the config
echo "Checking color definitions..."
rg -A 10 "colors.*:|theme.*colors.*:"
Length of output: 49876
internal/ui/src/components/button.tsx (2)
33-36: Empty class names for 'shape' variants may be unintended.
In the shape variants, both square and circle have empty class names, which means they don't apply any styles. If styles are intended for different shapes, consider adding the appropriate class names or removing the variants if they're not needed.
113-114: Ensure proper handling of props when using asChild with Slot.
When asChild is true, the component renders its children using Slot, which may not be a button element. Ensure that the child component accepts and correctly handles all necessary props like onClick, type, and disabled to maintain expected button behavior.
| @@ -5,6 +5,7 @@ const withMDX = createMDX(); | |||
| /** @type {import('next').NextConfig} */ | |||
| const config = { | |||
| reactStrictMode: true, | |||
| transpilePackages: ["@unkey/ui"], | |||
There was a problem hiding this comment.
💡 Codebase verification
Package dependency configuration needs attention
The @unkey/ui package is referenced in the Next.js configuration but appears to be missing:
- The workspace reference (
workspace:^) in apps/engineering/package.json suggests it should be a local workspace package - However, the package is not found in the packages directory
- This mismatch needs to be addressed before the transpilePackages configuration can work correctly
Consider one of the following actions:
- Add the missing
@unkey/uipackage to the packages directory - Update the package reference if the package name is incorrect
- Remove the transpilePackages configuration if the package is not needed
🔗 Analysis chain
LGTM! Verifying package dependency.
The transpilePackages configuration is correctly set up for @unkey/ui.
Let's verify the package dependency is properly configured:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify @unkey/ui package exists in package.json and node_modules
# Expected: Package should be listed in dependencies/devDependencies
# Check package.json for @unkey/ui
jq -r '.dependencies["@unkey/ui"] // .devDependencies["@unkey/ui"] // empty' apps/engineering/package.json
Length of output: 120
Script:
#!/bin/bash
# Let's check the root package.json and workspace configuration
cat package.json | jq -r '.workspaces // empty'
# Also check if the package exists in the monorepo
fd -t d "ui" packages/
Length of output: 103
| ], | ||
|
|
||
| theme: unkeyUiTailwindConfig.theme, |
There was a problem hiding this comment.
💡 Codebase verification
Theme inconsistency detected between dashboard and engineering apps
The dashboard app uses a custom theme configuration while the engineering app imports unkeyUiTailwindConfig.theme. This inconsistency could lead to design discrepancies across apps.
apps/dashboard/tailwind.config.js: Uses custom theme configurationapps/engineering/tailwind.config.js: UsesunkeyUiTailwindConfig.theme
🔗 Analysis chain
Verify theme consistency across apps
Using unkeyUiTailwindConfig.theme ensures consistent theming, which is crucial for a design system.
Let's check for other apps using the same theme configuration:
Let me check the content of the dashboard's tailwind config to verify if it needs the same theme configuration.
Let me check if there's a shared UI package that contains the unkeyUiTailwindConfig to understand the theme structure better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Tailwind configs that might need similar theme configuration
fd tailwind.config.js apps/ -x echo "Found Tailwind config: {}"
# Check if other configs are using the same theme
rg -l "unkeyUiTailwindConfig.theme" apps/
Length of output: 259
Script:
#!/bin/bash
# Check the content of dashboard's tailwind config
cat apps/dashboard/tailwind.config.js
Length of output: 3953
Script:
#!/bin/bash
# Search for the unkeyUiTailwindConfig definition
rg -l "unkeyUiTailwindConfig" --type ts --type js
# Also check for any package.json in packages directory that might contain UI components
fd package.json packages/ -x echo "Found package: {}" -x cat "{}"
Length of output: 701
| @@ -16,6 +16,12 @@ export const rfcs = defineCollections({ | |||
| type: "doc", | |||
| }); | |||
|
|
|||
| export const components = defineCollections({ | |||
| dir: "content/design", | |||
There was a problem hiding this comment.
💡 Codebase verification
The content/design directory needs to be created
The verification shows that while the configuration in source.config.ts points to content/design, this directory doesn't exist yet. The repository has multiple content directories under different apps (apps/engineering/content/, apps/www/content/), but none contain a design subdirectory. You'll need to:
- Create the
apps/engineering/content/designdirectory since the config file is in the engineering app - Ensure your design system documentation files are placed in this new directory
🔗 Analysis chain
Verify the content/design directory setup.
Let's ensure the directory structure is properly set up for the design system documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the content/design directory exists and has appropriate content
# Check if the directory exists and list its contents
if [ -d "content/design" ]; then
echo "Directory exists. Contents:"
ls -la content/design
else
echo "Warning: content/design directory not found"
fi
# Check for any existing MDX files that might need migration
fd -e mdx -e md . content/design
Length of output: 261
Script:
#!/bin/bash
# Let's check the repository structure to understand the content organization
fd --type directory content
fd --type directory docs
# Check if there are any existing design-related documentation files
rg -g "*.{md,mdx}" -l "design"
# Look for any configuration files that might reference content paths
rg -g "*.{ts,js,json}" "content.*design"
Length of output: 1083
| export const RenderComponentWithSnippet: React.FC<PropsWithChildren> = (props) => { | ||
| return ( | ||
| <div> | ||
| {props.children} | ||
| <CodeBlock> | ||
| <Pre> | ||
| {reactElementToJSXString(props.children, { | ||
| showFunctions: true, | ||
| useBooleanShorthandSyntax: true, | ||
| })} | ||
| </Pre> | ||
| </CodeBlock> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve type safety.
While the implementation is functional, consider these improvements for robustness:
- Add error boundaries to handle potential JSX conversion failures
- Add explicit return type annotation
- Consider using a more semantic wrapper with proper styling
Here's a suggested implementation:
-export const RenderComponentWithSnippet: React.FC<PropsWithChildren> = (props) => {
+export const RenderComponentWithSnippet: React.FC<PropsWithChildren> = (props): JSX.Element => {
+ const jsxString = React.useMemo(
+ () =>
+ reactElementToJSXString(props.children, {
+ showFunctions: true,
+ useBooleanShorthandSyntax: true,
+ }),
+ [props.children]
+ );
+
return (
- <div>
+ <section className="space-y-4">
{props.children}
- <CodeBlock>
- <Pre>
- {reactElementToJSXString(props.children, {
- showFunctions: true,
- useBooleanShorthandSyntax: true,
- })}
- </Pre>
- </CodeBlock>
- </div>
+ <ErrorBoundary fallback={<div>Failed to generate code snippet</div>}>
+ <CodeBlock>
+ <Pre>{jsxString}</Pre>
+ </CodeBlock>
+ </ErrorBoundary>
+ </section>
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const RenderComponentWithSnippet: React.FC<PropsWithChildren> = (props) => { | |
| return ( | |
| <div> | |
| {props.children} | |
| <CodeBlock> | |
| <Pre> | |
| {reactElementToJSXString(props.children, { | |
| showFunctions: true, | |
| useBooleanShorthandSyntax: true, | |
| })} | |
| </Pre> | |
| </CodeBlock> | |
| </div> | |
| ); | |
| }; | |
| export const RenderComponentWithSnippet: React.FC<PropsWithChildren> = (props): JSX.Element => { | |
| const jsxString = React.useMemo( | |
| () => | |
| reactElementToJSXString(props.children, { | |
| showFunctions: true, | |
| useBooleanShorthandSyntax: true, | |
| }), | |
| [props.children] | |
| ); | |
| return ( | |
| <section className="space-y-4"> | |
| {props.children} | |
| <ErrorBoundary fallback={<div>Failed to generate code snippet</div>}> | |
| <CodeBlock> | |
| <Pre>{jsxString}</Pre> | |
| </CodeBlock> | |
| </ErrorBoundary> | |
| </section> | |
| ); | |
| }; |
| @@ -0,0 +1,49 @@ | |||
| /** @type {import('tailwindcss').Config} */ | |||
| export default { | |||
| content: [], | |||
There was a problem hiding this comment.
Configure content paths for Tailwind processing
The empty content array will prevent Tailwind from processing any files. Based on the AI summary, you should include the UI component paths.
Apply this diff:
- content: [],
+ content: [
+ "../../internal/ui/**/*.tsx",
+ // Add other relevant paths where Tailwind classes are used
+ ],Committable suggestion skipped: line range outside the PR's diff.
| ## Keyboard | ||
|
|
||
| Providing a keyboard shortcut will override the `suffix` icon slot. | ||
|
|
||
| <RenderComponentWithSnippet> | ||
| <Row> | ||
| <Button variant="default" | ||
| keyboard={{ | ||
| display: "X", | ||
| trigger: (e)=>e.key==="x", | ||
| callback: ()=>{alert("hello keyboard warrior")} | ||
| }}>Search</Button> | ||
| <Button variant="primary" | ||
| keyboard={{ | ||
| display: "S", | ||
| trigger: (e)=>e.key==="s", | ||
| callback: ()=>{alert("hello keyboard warrior")} | ||
| }}>Search</Button> | ||
| <Button variant="ghost" | ||
| keyboard={{ | ||
| display: "F", | ||
| trigger: (e)=>e.key==="f", | ||
| callback: ()=>{alert("hello keyboard warrior")} | ||
| }}>Search</Button> | ||
| <Button variant="destructive" | ||
| keyboard={{ | ||
| display: "T", | ||
| trigger: (e)=>e.key==="t", | ||
| callback: ()=>{alert("hello keyboard warrior")} | ||
| }}>Search</Button> | ||
| </Row> | ||
| </RenderComponentWithSnippet> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix indentation and enhance keyboard shortcuts implementation
The keyboard shortcuts section has several areas for improvement:
- Inconsistent indentation in the keyboard configuration objects
- Using
alert()in callbacks - Single key triggers without modifier keys might conflict with browser shortcuts
keyboard={{
- display: "X",
- trigger: (e)=>e.key==="x",
- callback: ()=>{alert("hello keyboard warrior")}
+ display: "⌘+X",
+ trigger: (e) => e.key === "x" && e.metaKey,
+ callback: () => console.log("Keyboard shortcut triggered")
}}>Search</Button>Committable suggestion skipped: line range outside the PR's diff.
| ## info | ||
| <div className="grid grid-cols-12 gap-2"> | ||
| <div className="rounded-lg aspect-square bg-info-1"/> | ||
| <div className="rounded-lg aspect-square bg-info-2"/> | ||
| <div className="rounded-lg aspect-square bg-info-3"/> | ||
| <div className="rounded-lg aspect-square bg-info-4"/> | ||
| <div className="rounded-lg aspect-square bg-info-5"/> | ||
| <div className="rounded-lg aspect-square bg-info-6"/> | ||
| <div className="rounded-lg aspect-square bg-info-7"/> | ||
| <div className="rounded-lg aspect-square bg-info-8"/> | ||
| <div className="rounded-lg aspect-square bg-info-9"/> | ||
| <div className="rounded-lg aspect-square bg-info-10"/> | ||
| <div className="rounded-lg aspect-square bg-info-11"/> | ||
| <div className="rounded-lg aspect-square bg-info-12"/> | ||
| </div> |
There was a problem hiding this comment.
Remove duplicate "info" section
The "info" color grid is duplicated unnecessarily.
Remove the second occurrence of the info section (lines 80-94).
| <div className="grid grid-cols-12 gap-2"> | ||
| <div className="rounded-lg aspect-square bg-gray-1"/> | ||
| <div className="rounded-lg aspect-square bg-gray-2"/> | ||
| <div className="rounded-lg aspect-square bg-gray-3"/> | ||
| <div className="rounded-lg aspect-square bg-gray-4"/> | ||
| <div className="rounded-lg aspect-square bg-gray-5"/> | ||
| <div className="rounded-lg aspect-square bg-gray-6"/> | ||
| <div className="rounded-lg aspect-square bg-gray-7"/> | ||
| <div className="rounded-lg aspect-square bg-gray-8"/> | ||
| <div className="rounded-lg aspect-square bg-gray-9"/> | ||
| <div className="rounded-lg aspect-square bg-gray-10"/> | ||
| <div className="rounded-lg aspect-square bg-gray-11"/> | ||
| <div className="rounded-lg aspect-square bg-gray-12"/> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance color grid accessibility and maintainability
The color grids could benefit from several improvements:
- Add ARIA labels for screen readers
- Display color values (hex/rgb) on hover
- Consider extracting the repeated grid structure into a reusable component
Here's an example of how to improve one color grid:
-<div className="grid grid-cols-12 gap-2">
- <div className="rounded-lg aspect-square bg-gray-1"/>
- <div className="rounded-lg aspect-square bg-gray-2"/>
+<div className="grid grid-cols-12 gap-2" role="group" aria-label="Gray color scale">
+ <div
+ className="rounded-lg aspect-square bg-gray-1 group relative"
+ aria-label="Gray shade 1"
+ >
+ <span className="opacity-0 group-hover:opacity-100 absolute bottom-0 left-0 right-0 text-xs p-1 bg-black/50 text-white text-center">
+ Gray 1
+ </span>
+ </div>
+ <div
+ className="rounded-lg aspect-square bg-gray-2 group relative"
+ aria-label="Gray shade 2"
+ >
+ <span className="opacity-0 group-hover:opacity-100 absolute bottom-0 left-0 right-0 text-xs p-1 bg-black/50 text-white text-center">
+ Gray 2
+ </span>
+ </div>Consider creating a reusable component to reduce repetition:
type ColorSwatchProps = {
colorName: string;
shades: number[];
};
function ColorSwatchGrid({ colorName, shades }: ColorSwatchProps) {
return (
<div className="grid grid-cols-12 gap-2" role="group" aria-label={`${colorName} color scale`}>
{shades.map((shade) => (
<div
key={shade}
className={`rounded-lg aspect-square bg-${colorName}-${shade} group relative`}
aria-label={`${colorName} shade ${shade}`}
>
<span className="opacity-0 group-hover:opacity-100 absolute bottom-0 left-0 right-0 text-xs p-1 bg-black/50 text-white text-center">
{`${colorName} ${shade}`}
</span>
</div>
))}
</div>
);
}Also applies to: 49-62, 65-78, 96-110, 112-126, 128-142
| }; | ||
|
|
||
| const Button: React.FC<ButtonProps> = ({ className, variant, size, asChild = false, ...props }) => { | ||
| const ref = React.useRef<HTMLButtonElement>(); |
There was a problem hiding this comment.
ref type may be incompatible when asChild is true.
The ref is typed as React.useRef<HTMLButtonElement>(), but when asChild is true, Comp may render an element other than a button, and ref.current might not be an HTMLButtonElement. This could cause runtime errors when accessing ref.current?.focus().
Consider updating the ref type to accommodate different element types:
- const ref = React.useRef<HTMLButtonElement>();
+ const ref = React.useRef<HTMLElement>();And ensure any element rendered by Comp accepts the forwarded ref appropriately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ref = React.useRef<HTMLButtonElement>(); | |
| const ref = React.useRef<HTMLElement>(); |
| React.useEffect(() => { | ||
| if (!props.keyboard) { | ||
| return; | ||
| } | ||
| const down = (e: KeyboardEvent) => { | ||
| if (!props.keyboard!.trigger(e)) { | ||
| return; | ||
| } | ||
| e.preventDefault(); | ||
|
|
||
| ref.current?.focus(); | ||
| props.keyboard!.callback(e); | ||
| }; | ||
| document.addEventListener("keydown", down); | ||
| return () => document.removeEventListener("keydown", down); | ||
| }, [props.keyboard]); |
There was a problem hiding this comment.
Include all dependencies in useEffect to prevent stale references.
The useEffect dependency array currently includes only props.keyboard. If the trigger or callback functions within props.keyboard change, the effect won't re-run, potentially leading to stale closures. Include these functions in the dependency array to ensure the event listener stays up-to-date.
Apply this diff to update the dependency array:
}, [props.keyboard
+ , props.keyboard?.trigger, props.keyboard?.callback
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| React.useEffect(() => { | |
| if (!props.keyboard) { | |
| return; | |
| } | |
| const down = (e: KeyboardEvent) => { | |
| if (!props.keyboard!.trigger(e)) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| ref.current?.focus(); | |
| props.keyboard!.callback(e); | |
| }; | |
| document.addEventListener("keydown", down); | |
| return () => document.removeEventListener("keydown", down); | |
| }, [props.keyboard]); | |
| React.useEffect(() => { | |
| if (!props.keyboard) { | |
| return; | |
| } | |
| const down = (e: KeyboardEvent) => { | |
| if (!props.keyboard!.trigger(e)) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| ref.current?.focus(); | |
| props.keyboard!.callback(e); | |
| }; | |
| document.addEventListener("keydown", down); | |
| return () => document.removeEventListener("keydown", down); | |
| }, [props.keyboard, props.keyboard?.trigger, props.keyboard?.callback]); |
Summary by CodeRabbit
Release Notes
New Features
RenderComponentWithSnippetandRowcomponents for enhanced rendering and layout capabilities.componentSourceandcomponentsexports enhance modularity and documentation access.Bug Fixes
Documentation
Chores