docs(themes): add theme generator #3707
Conversation
|
|
@xylish7 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce a new navigation item in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![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
Outside diff range, codebase verification and nitpick comments (11)
apps/docs/components/themes/components/showcase/font-size.tsx (1)
1-14: LGTM! Consider adding more font sizes and line heights to showcase.The
FontSizecomponent is correctly implemented and showcases different font sizes using theShowcaseComponent. The class names used for thepelements are not defined in this file, but they are likely defined in a global CSS file or a CSS-in-JS library.Consider adding more font sizes and line heights to showcase, such as:
<div className="flex flex-col gap-2"> <p className="text-tiny">Tiny</p> <p className="text-small">Small</p> <p className="text-medium">Medium</p> <p className="text-large">Large</p> + <p className="text-xlarge">Extra Large</p> + <p className="text-2xlarge">2X Large</p> </div>apps/docs/components/themes/components/number-input.tsx (1)
11-21: Consider adding support for additional props and custom validation patterns.The component is correctly implemented and follows best practices by validating the input value. However, it can be improved by adding support for additional props and custom validation patterns.
Consider adding support for the following props:
min: The minimum value allowed.max: The maximum value allowed.step: The step value for incrementing/decrementing the value.precision: The number of decimal places to allow.pattern: A custom validation pattern to replace the defaultfloatNumberPattern.For example, the component props can be updated as follows:
interface NumberInputProps { label: string; value: string; onChange: (value: string) => void; min?: number; max?: number; step?: number; precision?: number; pattern?: RegExp; }Then, the
handleChangefunction can be updated to use the new props:function handleChange(event: React.ChangeEvent<HTMLInputElement>) { const value = event.target.value; const {min, max, step, precision, pattern} = props; if ((pattern || floatNumberPattern).test(value) || !value) { const newValue = Math.min(Math.max(Number(value), min ?? -Infinity), max ?? Infinity); const roundedValue = Number(newValue.toFixed(precision ?? 2)); onChange(roundedValue.toString()); } }Finally, the
Inputcomponent can be updated to use the new props:return ( <Input label={label} size="sm" value={value.toString()} onChange={handleChange} min={min} max={max} step={step} /> );apps/docs/components/themes/components/showcase/tabs.tsx (1)
15-21: Consider adding more tabs to eachNextUITabsinstance.To provide a more comprehensive showcase of the
NextUITabscomponent, consider adding more tabs to eachNextUITabsinstance. This will help users better understand how the component behaves with different numbers of tabs.For example, you can add more tabs like this:
<NextUITabs key={color} aria-label="Tabs colors" color={color} radius="full"> <Tab key="photos" title="Photos" /> <Tab key="music" title="Music" /> <Tab key="videos" title="Videos" /> + <Tab key="documents" title="Documents" /> + <Tab key="downloads" title="Downloads" /> </NextUITabs>apps/docs/components/themes/components/showcase/content.tsx (1)
6-37: Consider extracting the repeated card structure into a separate sub-component.The
Contentcomponent contains a repeated structure for rendering the content color cards. To improve readability and maintainability, consider extracting this repeated structure into a separate sub-component.Here's an example of how you can extract the card structure:
const ContentCard: React.FC<{ className?: string; title: string; description: string }> = ({ className, title, description, }) => ( <Card className={className}> <CardBody> <p>{title}</p> <p>{description}</p> </CardBody> </Card> ); export function Content() { return ( <ShowcaseComponent id={contentShowcaseId} name="Content colors"> <div className="grid grid-cols-2 lg:grid-cols-4 gap-4"> <ContentCard title="Content 1" description="bg-content1 text-content-foreground1" /> <ContentCard className="bg-content2 text-content2-foreground" title="Content 2" description="bg-content2 text-content-foreground2" /> <ContentCard className="bg-content3 text-content3-foreground" title="Content 3" description="bg-content3 text-content-foreground3" /> <ContentCard className="bg-content4 text-content4-foreground" title="Content 4" description="bg-content4 text-content-foreground4" /> </div> </ShowcaseComponent> ); }This refactoring enhances the code by:
- Reducing duplication and improving maintainability.
- Making the
Contentcomponent more focused and readable.- Providing a reusable
ContentCardcomponent for future use.apps/docs/components/themes/components/showcase/index.tsx (1)
23-47: LGTM!The code changes are approved.
Nitpick: Consider adding a comment to explain the purpose of the
Showcasecomponent.To improve the maintainability and readability of the code, consider adding a brief comment above the
Showcasecomponent to explain its purpose and the role of the rendered UI components.Here's an example:
+/** + * Showcase component that renders various UI components for theme preview. + */ export function Showcase() { return ( <div className="grid grid-cols-1 gap-4 w-full " id={showcaseId}> {/* Rendered UI components */} </div> ); }apps/docs/components/themes/components/showcase/badge.tsx (1)
6-29: LGTM!The
Badgecomponent provides a clear and concise way to showcase theNextUIBadgecomponent with different colors. The use of theShowcaseComponenthelps to keep the code DRY and maintainable.Consider using a placeholder image or a more reliable source for avatar images in a production environment. The hardcoded
srcprop of theNextUIAvatarcomponent is acceptable for a showcase component, but it may not be suitable for a production environment.apps/docs/components/themes/components/select-template.tsx (2)
11-40: Simplify thehandleChangefunction and improve theSelectandSelectItemcomponents.Consider the following improvements:
- Simplify the
handleChangefunction by using thefindmethod directly on thetemplatesarray:-function handleChange(e: React.ChangeEvent<HTMLSelectElement>) { - const value = e.target.value as TemplateType; - const template = templates.find((template) => template.name === value); - - if (template) { - onChange(template); - } -} +function handleChange(value: string) { + const template = templates.find((template) => template.name === value); + onChange(template!); +}
- Simplify the
selectedKeysprop of theSelectcomponent by using a ternary operator:-selectedKeys={name === null ? [] : [name]} +selectedKeys={name ? [name] : []}
- Set the
valueprop of theSelectItemcomponent to thenameof the template instead of the index:-<SelectItem key={template.name} value={index}> +<SelectItem key={template.name} value={template.name}>
46-54: Simplify the component and improve accessibility.Consider the following improvements:
- Simplify the component by using the
Object.valuesmethod instead ofObject.entries:-{Object.entries(colors).map(([key, value]) => ( - <div key={key} className="w-2 h-full" style={{background: value}} /> -))} +{Object.values(colors).map((value, index) => ( + <div key={index} className="w-2 h-full" style={{background: value}} /> +))}
- Add an
aria-labelto thedivelements to improve accessibility:-<div key={index} className="w-2 h-full" style={{background: value}} /> +<div key={index} className="w-2 h-full" style={{background: value}} aria-label={`Color ${index + 1}`} />apps/docs/components/themes/components/configuration/actions.tsx (2)
30-30: Consider using a constant for the timeout duration.To improve readability and maintainability, consider defining a constant for the timeout duration used in the
setTimeoutfunction.Apply this diff to define a constant for the timeout duration:
+const COPIED_TIMEOUT_MS = 1500; function handleCopyConfig() { navigator.clipboard.writeText(JSON.stringify(onCopy(), null, 2)); setCopied(true); - setTimeout(() => setCopied(false), 1500); + setTimeout(() => setCopied(false), COPIED_TIMEOUT_MS); }
36-36: Consider adding aria-labels to the buttons for accessibility.To improve accessibility, consider adding
aria-labelattributes to the buttons.Apply this diff to add
aria-labelattributes to the buttons:<Tooltip content={isLight ? "Dark" : "Light"}> - <Button isIconOnly color="secondary" size="sm" variant="flat" onClick={onToggleTheme}> + <Button isIconOnly color="secondary" size="sm" variant="flat" onClick={onToggleTheme} aria-label="Toggle theme"> {isLight ? ( <Icon className="text-lg" icon={MoonIcon} /> ) : ( <Icon className="text-lg" icon={SunIcon} /> )} </Button> </Tooltip> <Tooltip content="Reset theme"> - <Button isIconOnly color="secondary" size="sm" variant="flat" onClick={onResetTheme}> + <Button isIconOnly color="secondary" size="sm" variant="flat" onClick={onResetTheme} aria-label="Reset theme"> <Icon className="text-lg" icon={UndoLeftIcon} /> </Button> </Tooltip> <Tooltip content="Copy configuration"> - <Button isIconOnly color="secondary" size="sm" variant="flat" onClick={handleCopyConfig}> + <Button isIconOnly color="secondary" size="sm" variant="flat" onClick={handleCopyConfig} aria-label="Copy configuration"> {copied ? ( <Icon className="text-lg" icon={CheckCircleIcon} /> ) : ( <Icon className="text-lg" icon={CopyIcon} /> )} </Button> </Tooltip>Also applies to: 45-45, 50-50
apps/docs/components/themes/components/configuration/brand-colors.tsx (1)
22-68: Consider extracting the repeated color picker configurations into a separate component or a loop.The component contains repeated configurations for each color picker, which can be extracted into a separate component or rendered using a loop to reduce code duplication and improve maintainability.
Here's an example of how you can extract the color picker configurations into a separate component:
interface ColorPickerConfigProps { hexColor: string; icon?: React.ReactNode; label: string; type: string; onChange: (hexColor: string) => void; onClose: (hexColor: string) => void; } function ColorPickerConfig({hexColor, icon, label, type, onChange, onClose}: ColorPickerConfigProps) { return ( <ColorPicker hexColor={hexColor} icon={icon} label={label} type={type} onChange={onChange} onClose={onClose} /> ); }Then, you can render the color pickers using a loop or by calling the
ColorPickerConfigcomponent for each brand color type:<ConfigSection id={colorsId} title="Brand colors"> {["default", "primary", "secondary", "success", "warning", "danger"].map((type) => ( <ColorPickerConfig key={type} hexColor={config[theme].brandColor[type]} icon={type !== "default" ? syncIcon : undefined} label={capitalize(type)} type={type} onChange={(hexColor) => setCssColor(type, hexColor, theme)} onClose={(hexColor) => setBrandColor({[type]: hexColor}, theme, syncThemes)} /> ))} </ConfigSection>This approach reduces code duplication and makes the component more maintainable.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (52)
- apps/docs/app/themes/page.tsx (1 hunks)
- apps/docs/components/navbar.tsx (1 hunks)
- apps/docs/components/themes/components/color-picker.tsx (1 hunks)
- apps/docs/components/themes/components/config-section.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/actions.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/base-colors.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/border-widths.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/brand-colors.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/font-sizes.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/index.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/line-heights.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/other-colors.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/other.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/radiuses.tsx (1 hunks)
- apps/docs/components/themes/components/copy-button.tsx (1 hunks)
- apps/docs/components/themes/components/number-input.tsx (1 hunks)
- apps/docs/components/themes/components/select-template.tsx (1 hunks)
- apps/docs/components/themes/components/showcase-component.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/avatar.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/badge.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/breadcrumbs.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/button.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/checkbox.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/chip.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/circular-progress.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/code.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/content.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/divider.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/dropdown.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/font-size.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/index.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/input.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/line-height.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/link.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/pagination.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/select.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/slider.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/switch.tsx (1 hunks)
- apps/docs/components/themes/components/showcase/tabs.tsx (1 hunks)
- apps/docs/components/themes/constants.ts (1 hunks)
- apps/docs/components/themes/css-vars.ts (1 hunks)
- apps/docs/components/themes/index.tsx (1 hunks)
- apps/docs/components/themes/provider.tsx (1 hunks)
- apps/docs/components/themes/templates/coffee.ts (1 hunks)
- apps/docs/components/themes/templates/emerald.ts (1 hunks)
- apps/docs/components/themes/templates/index.ts (1 hunks)
- apps/docs/components/themes/templates/nextui.ts (1 hunks)
- apps/docs/components/themes/types.ts (1 hunks)
- apps/docs/components/themes/utils/colors.ts (1 hunks)
- apps/docs/components/themes/utils/config.ts (1 hunks)
- apps/docs/hooks/use-previous.ts (1 hunks)
- apps/docs/package.json (2 hunks)
Files skipped from review due to trivial changes (1)
- apps/docs/components/themes/components/showcase/line-height.tsx
Additional comments not posted (89)
apps/docs/app/themes/page.tsx (1)
1-9: LGTM!The code changes are approved for the following reasons:
- The component is correctly implemented.
- The component name and the file name match, following the best practices.
- The component is using a default export, which is a common practice in Next.js.
- The styling classes are using Tailwind CSS utility classes, which is a popular CSS framework.
apps/docs/components/themes/templates/nextui.ts (1)
1-8: LGTM!The code changes are approved for the following reasons:
- The file is a template for the NextUI theme configuration.
- The configuration object is correctly structured and initialized with the appropriate values from the
constantsmodule.- The file is small and focused on a single responsibility, which is good for maintainability.
apps/docs/components/themes/index.tsx (1)
1-14: LGTM!The
ThemeBuildercomponent is well-structured and follows the provider pattern. It properly wraps theConfigurationandShowcasecomponents with theThemeBuilderProvidercomponent, allowing them to access the common context.The use of the "use client" directive is correct for a client component in a Next.js app directory.
The code changes are approved.
apps/docs/components/themes/templates/index.ts (4)
1-1: LGTM!The import statement is correctly used to import the required
Templatetype.
3-5: LGTM!The import statements are correctly used to import the required constants.
7-11: LGTM!The
templatesarray is correctly defined and exported. The use of object literals to define each template is a clean and readable approach. Thevalueproperty is correctly assigned the imported constants.
Line range hint
12-12: LGTM!The empty line at the end of the file is a good practice for improving readability.
apps/docs/hooks/use-previous.ts (1)
1-18: LGTM!The
usePrevioushook is a well-implemented custom hook that follows the best practices for React hooks. It provides a useful functionality to store and retrieve the previous value of a given parameter.Some key observations:
- The hook is correctly typed with a generic type
T, allowing it to be used with any type of value.- The JSDoc comments provide a clear description of the hook's purpose and parameters.
- The
useRefhook is used to store the previous value, and theuseEffecthook is used to update the ref whenever the value changes.- The
useEffecthook has the correct dependency array, ensuring that the ref is updated only when the value changes.Overall, the code is clean, concise, and follows the best practices for custom hooks in React.
apps/docs/components/themes/components/config-section.tsx (1)
1-24: LGTM!The
ConfigSectioncomponent is well-implemented and follows best practices. It provides a reusable and customizable solution for rendering configuration sections.Observations:
- The component is properly typed with an interface for its props.
- It uses the
clsxutility for managing conditional class names, which is a good approach.- The component is structured in a clean and readable manner.
- It accepts props for customization, such as the number of columns and an optional ID.
The code changes are approved.
apps/docs/components/themes/components/copy-button.tsx (1)
1-26: LGTM!The
CopyButtoncomponent is well-structured, follows best practices, and is reusable. The code changes are approved.apps/docs/components/themes/components/showcase/chip.tsx (1)
1-20: LGTM!The code is well-structured and follows best practices. The
Chipcomponent effectively showcases the different variants and sizes of the NextUI Chip component. Thesizesandvariantsarrays are defined as constants, which is a good practice.The code changes are approved.
apps/docs/components/themes/components/showcase/divider.tsx (1)
1-26: LGTM!The
Dividercomponent is well-implemented and effectively showcases the usage of theNextUIDividercomponent. The code is properly structured, uses the imports correctly, and has valid JSX. The Tailwind CSS classes are used appropriately for styling. I don't see any issues or areas for improvement.Great job! The code changes are approved.
apps/docs/components/themes/components/showcase/circular-progress.tsx (1)
1-19: LGTM!The code is well-structured and follows best practices. The
CircularProgresscomponent effectively showcases the different color and size variants of theNextUICircularProgresscomponent. The use of theShowcaseComponenthelps to maintain consistency with other showcase components. Thesizesconstant is appropriately typed using theNextUISizetype.apps/docs/components/themes/components/showcase/link.tsx (1)
1-31: LGTM!The
Linkcomponent is well-structured and effectively demonstrates the usage of theNextUILinkcomponent with different colors and sizes. The code is clean, readable, and follows best practices.apps/docs/components/themes/components/showcase/input.tsx (1)
6-22: LGTM!The code changes are approved.
apps/docs/components/themes/components/showcase/code.tsx (1)
6-17: LGTM!The
Codecomponent is implemented correctly and showcases theNextUICodecomponent effectively. The use of theShowcaseComponentprovides a consistent layout and styling, and the different colors demonstrate the available color options. Theradiusesandsizesarrays are passed as props to control the available radius and size options.apps/docs/components/themes/components/showcase/slider.tsx (1)
6-23: LGTM!The
Slidercomponent is implemented correctly and showcases theNextUISlidercomponent with different variations. The code follows good practices such as using thearia-labelattribute for accessibility and thekeyprop for unique identification of rendered components. The handling of the default color and the values fordefaultValue,maxValue,minValue, andstepprops are appropriate.The code changes are approved.
apps/docs/components/themes/components/showcase/switch.tsx (1)
1-31: LGTM!The code looks good and follows best practices:
- The imports are properly named and organized.
- The
Switchcomponent showcases the different color variants of theNextUISwitchcomponent.- The
sizesconstant is used to define the sizes for theShowcaseComponent.- The code is well-structured and readable.
Great job!
apps/docs/components/themes/components/showcase/button.tsx (1)
1-35: LGTM!The
Buttoncomponent is a good example of how to use theShowcaseComponentcomponent to display components with different properties. The component is correctly implemented and follows the best practices.The component uses the
NextUIButtoncomponent from the@nextui-org/reactlibrary to display the buttons and theNextUIRadius,NextUISize, andNextUIVarianttypes from the../../typesmodule to define the radiuses, sizes, and variants of the buttons.The code changes are approved.
apps/docs/components/themes/components/showcase/checkbox.tsx (1)
1-32: LGTM!The code for the
Checkboxshowcase component looks good:
- It properly imports the necessary components and types.
- The component structure is clear and easy to understand.
- It showcases the different variants of the NextUI Checkbox component with various colors, radiuses, and sizes.
- The use of the
ShowcaseComponenthelps in organizing and presenting the checkbox variants effectively.- The
radiusesandsizesarrays provide a clean separation of the available customization options.Overall, the code follows best practices and contributes to the theme builder's functionality.
apps/docs/components/themes/templates/emerald.ts (1)
1-44: LGTM!The code changes are approved for the following reasons:
- The file is well-structured and follows a consistent pattern for defining the theme configuration.
- The color palette values are hardcoded, which is acceptable for a predefined theme template.
- The theme configuration leverages the initial theme constants to ensure consistency with the default themes.
- The file does not have any obvious issues or code smells.
apps/docs/components/themes/components/showcase/breadcrumbs.tsx (3)
1-4: LGTM!The code changes are approved.
6-28: LGTM!The code changes are approved.
30-32: LGTM!The code changes are approved.
apps/docs/components/themes/components/configuration/radiuses.tsx (1)
1-44: LGTM!The
Radiusescomponent is well-structured and follows best practices. It effectively uses theThemeBuilderContextto manage the theme configuration and updates the radius values correctly. The component is modular and can be easily integrated into the theme builder.The code changes are approved.
apps/docs/components/themes/components/showcase/index.tsx (1)
1-22: LGTM!The code changes are approved.
apps/docs/components/themes/templates/coffee.ts (1)
1-54: LGTM!The code changes are approved.
apps/docs/components/themes/components/showcase/pagination.tsx (3)
1-5: LGTM!The code changes are approved.
6-35: LGTM!The code changes are approved.
37-39: LGTM!The code changes are approved.
apps/docs/components/themes/components/showcase/badge.tsx (1)
31-32: LGTM!The
sizesandvariantsarrays provide a clear separation of concerns and make it easy to modify the available options for theNextUIBadgecomponent. Defining the arrays as constants outside theBadgecomponent helps to keep the component focused on rendering theNextUIBadgecomponents and makes it easy to reuse the arrays in other parts of the codebase if needed.apps/docs/components/themes/components/configuration/font-sizes.tsx (1)
13-52: LGTM!The
FontSizescomponent is well-implemented:
- It correctly uses the
ThemeBuilderContextto manage the font sizes.- It renders a
ConfigSectionwithNumberInputcomponents for each font size.- The
NumberInputcomponents are properly bound to theconfigprop.- The
onChangehandlers correctly update the context and CSS variables.The code changes are approved.
apps/docs/components/themes/components/configuration/line-heights.tsx (1)
1-52: LGTM!The
LineHeightscomponent is well-structured and follows best practices:
- It is typed with an interface for props.
- It uses the
useContexthook to access the theme context.- It renders a section with input components for each line height size.
- The
onChangehandlers update the context and CSS variables.The code changes are approved.
apps/docs/components/themes/components/showcase/avatar.tsx (1)
1-52: LGTM!The
Avatarcomponent is well-structured and serves its purpose of showcasing different color variants of theNextUIAvatarcomponent. The use of theShowcaseComponentand theradiusesandsizesarrays helps to provide a consistent and customizable showcase. The code is readable and easy to understand.The code changes are approved.
apps/docs/components/themes/components/showcase/select.tsx (1)
1-60: LGTM!The
Selectcomponent is well-structured and follows best practices for React components. It showcases the select component with different configurations using theShowcaseComponent. The component uses a good approach to render the options for the select component and to limit its width. It also sets a default selected value, a label, and a placeholder for the select component. The component passes the configurations as props to theShowcaseComponentand imports the required types and components from other files.The code changes are approved.
apps/docs/components/themes/components/showcase/dropdown.tsx (1)
1-48: LGTM!The code changes in this new file are well-structured, modular, and follow best practices. The
DropdownandDropdownContentcomponents are separated, allowing for reusability and customization. The use of theShowcaseComponentand theNextUIDropdowncomponent from the@nextui-org/reactlibrary ensures consistency with the library's design system.The code imports necessary components and types, and the
colorandvariantprops are used appropriately to customize the appearance of the dropdown.Overall, the code is clean, readable, and maintainable.
apps/docs/components/themes/components/configuration/other-colors.tsx (2)
17-46: LGTM!The
OtherColorscomponent is well-structured and correctly implemented:
- It uses the
useContexthook to access theThemeBuilderContext.- It renders a
ConfigSectioncomponent to group the color pickers.- The
ColorPickercomponents are correctly configured with the current color values, labels, and change handlers.- The
onChangeandonClosehandlers are correctly updating the CSS variables and the theme configuration.
10-15: LGTM!The
OtherColorsPropsinterface correctly specifies the props and their types for theOtherColorscomponent.apps/docs/components/themes/components/configuration/actions.tsx (1)
1-60: LGTM!The code changes are approved. The component is well-structured and follows best practices for React components.
apps/docs/components/themes/components/configuration/base-colors.tsx (1)
15-64: LGTM!The
BaseColorscomponent is well-structured and follows best practices:
- It uses the
ThemeBuilderContextappropriately for accessing and updating the theme configuration.- The
ColorPickercomponents are used consistently for each base color.- The
onChangeandonClosecallbacks update the CSS variables and the theme configuration correctly.- The component is modular and reusable.
The code changes are approved.
apps/docs/components/themes/types.ts (4)
2-13: LGTM!The
ColorShadesinterface is well-defined and follows a common convention for defining color shades.
15-30: LGTM!The
ColorPickerTypetype is well-defined and covers a comprehensive set of color picker types.
32-45: LGTM!The NextUI component prop types are well-defined and cover the necessary options for NextUI colors, sizes, variants, and radii.
47-121: LGTM!The theme-related types and interfaces are well-structured and cover the necessary aspects of theme configuration. The separation of light and dark theme colors, layout configurations, and the ability to define predefined theme templates provide a solid foundation for theme management.
apps/docs/components/themes/components/configuration/brand-colors.tsx (1)
1-71: LGTM!The
BrandColorscomponent is well-structured and correctly implements the functionality for configuring brand colors in the theme builder. The usage of context and props is appropriate, and the color picker configurations are consistent and mapped correctly to the corresponding brand color types.apps/docs/components/themes/constants.ts (6)
1-3: LGTM!The code changes are approved.
6-8: LGTM!The code changes are approved.
11-11: LGTM!The code changes are approved.
14-18: LGTM!The code changes are approved.
21-22: LGTM!The code changes are approved.
25-107: LGTM!The code changes are approved.
apps/docs/components/themes/utils/colors.ts (6)
11-13: LGTM!The code changes are approved.
18-32: LGTM!The code changes are approved.
37-82: LGTM!The code changes are approved.
87-93: LGTM!The code changes are approved.
107-109: LGTM!The code changes are approved.
114-133: LGTM!The code changes are approved.
apps/docs/package.json (2)
75-75: LGTM!The addition of the
react-colorfuldependency is approved. It provides a color picker component for React applications, which aligns with the project's requirements for color selection functionality. The version constraint^5.6.1is also reasonable.
99-99: LGTM!The addition of the
values.jsdependency is approved. It provides utilities for managing color values and conversions, which aligns with the project's requirements for advanced color handling capabilities. The version constraint^2.1.1is also reasonable.apps/docs/components/themes/components/showcase-component.tsx (4)
16-96: LGTM!The
ShowcaseComponentis well-implemented and follows best practices. The use ofuseStatefor state management,cloneElementfor applying props to children, and conditional rendering of customization options based on the presence of corresponding props is appreciated. The component also maintains consistency with the rest of the codebase by using a combination of Tailwind CSS classes and NextUI components for styling.
98-104: LGTM!The
defaultRadiusesconstant is correctly typed and initialized. The radius options are comprehensive, covering common use cases.
106-116: LGTM!The
defaultVariantsconstant is correctly typed and initialized. The variant options are comprehensive, covering common use cases.
118-122: LGTM!The
defaultSizesconstant is correctly typed and initialized. The size options are comprehensive, covering common use cases.apps/docs/components/themes/components/color-picker.tsx (2)
22-92: LGTM!The
ColorPickercomponent is well-implemented and uses appropriate libraries for color picking and manipulation. The code is structured, readable, and follows best practices. The component correctly handles user interactions and updates the selected color when necessary.
94-129: LGTM!The
getColorfunction is a well-implemented utility function that maps theColorPickerTypeto the corresponding CSS class name. The function uses a switch statement to handle the different cases and returns the appropriate CSS class name based on the input. The function handles all possible cases and returnsundefinedfor the default case.apps/docs/components/themes/components/configuration/index.tsx (1)
1-127: Excellent work on the Configuration component!The component is well-structured, modular, and effectively manages the theme configuration using the
ThemeBuilderContext. It handles theme-related actions cleanly and provides a comprehensive set of theme configuration sections for a user-friendly customization experience.Some key highlights:
- Clear separation of concerns with well-defined sub-components for each configuration section.
- Effective utilization of the
ThemeBuilderContextfor accessing and modifying the theme configuration.- Clean and readable handling of theme syncing, toggling, and resetting actions.
- Comprehensive set of theme configuration sections rendered in a user-friendly card layout.
Overall, the code changes are of high quality and align well with best practices for React component development.
apps/docs/components/themes/utils/config.ts (1)
12-116: LGTM!The
generatePluginConfigfunction is well-structured, modular, and follows best practices:
- It uses destructuring to extract values from the
Configobject, which improves readability.- It uses template literals to generate CSS values, which is a good practice.
- It uses the
readableColorfunction to generate foreground colors for content colors, which improves accessibility.- It uses the
generateThemeColorfunction to generate color values based on the provided brand and base colors, which is a good abstraction.- It returns an object with the generated
layoutandthemesobjects, which is a good way to structure the plugin configuration.The code changes are approved.
apps/docs/components/themes/css-vars.ts (8)
31-38: LGTM!The code changes are approved.
40-44: LGTM!The code changes are approved.
46-50: LGTM!The code changes are approved.
52-56: LGTM!The code changes are approved.
58-62: LGTM!The code changes are approved.
64-79: LGTM!The code changes are approved.
81-88: LGTM!The code changes are approved.
90-106: LGTM!The code changes are approved.
apps/docs/components/themes/provider.tsx (13)
48-48: There is only one local storage related import.The file is only using the
useLocalStoragehook from theusehooks-tslibrary. There are no other local storage related imports in the file. Therefore, you should continue using theuseLocalStoragehook.
52-61: LGTM!The code changes are approved.
63-79: LGTM!The code changes are approved.
81-115: LGTM!The code changes are approved.
117-131: LGTM!The code changes are approved.
133-167: LGTM!The code changes are approved.
169-182: LGTM!The code changes are approved.
184-197: LGTM!The code changes are approved.
199-212: LGTM!The code changes are approved.
214-227: LGTM!The code changes are approved.
229-242: LGTM!The code changes are approved.
244-271: LGTM!The code changes are approved.
273-275: LGTM!The code changes are approved.
apps/docs/components/navbar.tsx (1)
239-249: LGTM!The new
NavbarItemcomponent for the "Themes" section is implemented correctly:
- The
NextLinkcomponent links to the correct "/themes" route.- The link is styled appropriately using the
navLinkClassesand is conditionally marked as active based on the current pathname.- The
onClickhandler tracks user interactions with this new item, providing valuable analytics data.These changes enhance the navigation functionality by providing users with direct access to the themes section, thereby improving the overall user experience.
| export function BorderWidths({config}: BorderWidthsProps) { | ||
| const {setBorderWidth} = useContext(ThemeBuilderContext); | ||
|
|
||
| return ( | ||
| <ConfigSection cols={3} title="Border width (px)"> | ||
| <NumberInput | ||
| label="Small" | ||
| value={config.layout.borderWidth.small} | ||
| onChange={(value) => { | ||
| setBorderWidth({small: value}); | ||
| setCssBorderWidth("small", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Medium" | ||
| value={config.layout.borderWidth.medium} | ||
| onChange={(value) => { | ||
| setBorderWidth({medium: value}); | ||
| setCssBorderWidth("medium", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Large" | ||
| value={config.layout.borderWidth.large} | ||
| onChange={(value) => { | ||
| setBorderWidth({large: value}); | ||
| setCssBorderWidth("large", value); | ||
| }} | ||
| /> | ||
| </ConfigSection> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider extracting the onChange handler to a separate function.
The onChange handlers for the NumberInput components are duplicated. Consider extracting the handler to a separate function to avoid duplication and improve maintainability.
Apply this diff to extract the onChange handler:
+const handleBorderWidthChange = (key: keyof Config["layout"]["borderWidth"]) => (value: number) => {
+ setBorderWidth({[key]: value});
+ setCssBorderWidth(key, value);
+};
+
return (
<ConfigSection cols={3} title="Border width (px)">
<NumberInput
label="Small"
value={config.layout.borderWidth.small}
- onChange={(value) => {
- setBorderWidth({small: value});
- setCssBorderWidth("small", value);
- }}
+ onChange={handleBorderWidthChange("small")}
/>
<NumberInput
label="Medium"
value={config.layout.borderWidth.medium}
- onChange={(value) => {
- setBorderWidth({medium: value});
- setCssBorderWidth("medium", value);
- }}
+ onChange={handleBorderWidthChange("medium")}
/>
<NumberInput
label="Large"
value={config.layout.borderWidth.large}
- onChange={(value) => {
- setBorderWidth({large: value});
- setCssBorderWidth("large", value);
- }}
+ onChange={handleBorderWidthChange("large")}
/>
</ConfigSection>
);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 function BorderWidths({config}: BorderWidthsProps) { | |
| const {setBorderWidth} = useContext(ThemeBuilderContext); | |
| return ( | |
| <ConfigSection cols={3} title="Border width (px)"> | |
| <NumberInput | |
| label="Small" | |
| value={config.layout.borderWidth.small} | |
| onChange={(value) => { | |
| setBorderWidth({small: value}); | |
| setCssBorderWidth("small", value); | |
| }} | |
| /> | |
| <NumberInput | |
| label="Medium" | |
| value={config.layout.borderWidth.medium} | |
| onChange={(value) => { | |
| setBorderWidth({medium: value}); | |
| setCssBorderWidth("medium", value); | |
| }} | |
| /> | |
| <NumberInput | |
| label="Large" | |
| value={config.layout.borderWidth.large} | |
| onChange={(value) => { | |
| setBorderWidth({large: value}); | |
| setCssBorderWidth("large", value); | |
| }} | |
| /> | |
| </ConfigSection> | |
| ); | |
| } | |
| export function BorderWidths({config}: BorderWidthsProps) { | |
| const {setBorderWidth} = useContext(ThemeBuilderContext); | |
| const handleBorderWidthChange = (key: keyof Config["layout"]["borderWidth"]) => (value: number) => { | |
| setBorderWidth({[key]: value}); | |
| setCssBorderWidth(key, value); | |
| }; | |
| return ( | |
| <ConfigSection cols={3} title="Border width (px)"> | |
| <NumberInput | |
| label="Small" | |
| value={config.layout.borderWidth.small} | |
| onChange={handleBorderWidthChange("small")} | |
| /> | |
| <NumberInput | |
| label="Medium" | |
| value={config.layout.borderWidth.medium} | |
| onChange={handleBorderWidthChange("medium")} | |
| /> | |
| <NumberInput | |
| label="Large" | |
| value={config.layout.borderWidth.large} | |
| onChange={handleBorderWidthChange("large")} | |
| /> | |
| </ConfigSection> | |
| ); | |
| } |
| export function Other({config}: OtherProps) { | ||
| const {setOtherParams} = useContext(ThemeBuilderContext); | ||
|
|
||
| return ( | ||
| <ConfigSection cols={1} title="Other"> | ||
| <NumberInput | ||
| label="Disabled opacity (0-1)" | ||
| value={config.layout.otherParams.disabledOpacity} | ||
| onChange={(value) => { | ||
| setOtherParams({disabledOpacity: value}); | ||
| setOtherCssParams("disabledOpacity", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Divider weight (px)" | ||
| value={config.layout.otherParams.dividerWeight} | ||
| onChange={(value) => { | ||
| setOtherParams({dividerWeight: value}); | ||
| setOtherCssParams("dividerWeight", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Hover opacity (0-1)" | ||
| value={config.layout.otherParams.hoverOpacity} | ||
| onChange={(value) => { | ||
| setOtherParams({hoverOpacity: value}); | ||
| setOtherCssParams("hoverOpacity", value); | ||
| }} | ||
| /> | ||
| </ConfigSection> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider extracting the configuration options to a separate object.
To make the component more reusable and maintainable, you can extract the configuration options to a separate object. This will allow you to easily add or remove options without modifying the component code.
Here's an example of how you can refactor the code:
+const configOptions = [
+ {
+ label: 'Disabled opacity (0-1)',
+ key: 'disabledOpacity',
+ },
+ {
+ label: 'Divider weight (px)',
+ key: 'dividerWeight',
+ },
+ {
+ label: 'Hover opacity (0-1)',
+ key: 'hoverOpacity',
+ },
+];
+
export function Other({config}: OtherProps) {
const {setOtherParams} = useContext(ThemeBuilderContext);
+ const handleChange = (key: keyof Config['layout']['otherParams'], value: number) => {
+ setOtherParams({ [key]: value });
+ setOtherCssParams(key, value);
+ };
+
return (
<ConfigSection cols={1} title="Other">
- <NumberInput
- label="Disabled opacity (0-1)"
- value={config.layout.otherParams.disabledOpacity}
- onChange={(value) => {
- setOtherParams({disabledOpacity: value});
- setOtherCssParams("disabledOpacity", value);
- }}
- />
- <NumberInput
- label="Divider weight (px)"
- value={config.layout.otherParams.dividerWeight}
- onChange={(value) => {
- setOtherParams({dividerWeight: value});
- setOtherCssParams("dividerWeight", value);
- }}
- />
- <NumberInput
- label="Hover opacity (0-1)"
- value={config.layout.otherParams.hoverOpacity}
- onChange={(value) => {
- setOtherParams({hoverOpacity: value});
- setOtherCssParams("hoverOpacity", value);
- }}
- />
+ {configOptions.map(({label, key}) => (
+ <NumberInput
+ key={key}
+ label={label}
+ value={config.layout.otherParams[key]}
+ onChange={(value) => handleChange(key, value)}
+ />
+ ))}
</ConfigSection>
);
}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 function Other({config}: OtherProps) { | |
| const {setOtherParams} = useContext(ThemeBuilderContext); | |
| return ( | |
| <ConfigSection cols={1} title="Other"> | |
| <NumberInput | |
| label="Disabled opacity (0-1)" | |
| value={config.layout.otherParams.disabledOpacity} | |
| onChange={(value) => { | |
| setOtherParams({disabledOpacity: value}); | |
| setOtherCssParams("disabledOpacity", value); | |
| }} | |
| /> | |
| <NumberInput | |
| label="Divider weight (px)" | |
| value={config.layout.otherParams.dividerWeight} | |
| onChange={(value) => { | |
| setOtherParams({dividerWeight: value}); | |
| setOtherCssParams("dividerWeight", value); | |
| }} | |
| /> | |
| <NumberInput | |
| label="Hover opacity (0-1)" | |
| value={config.layout.otherParams.hoverOpacity} | |
| onChange={(value) => { | |
| setOtherParams({hoverOpacity: value}); | |
| setOtherCssParams("hoverOpacity", value); | |
| }} | |
| /> | |
| </ConfigSection> | |
| ); | |
| } | |
| const configOptions = [ | |
| { | |
| label: 'Disabled opacity (0-1)', | |
| key: 'disabledOpacity', | |
| }, | |
| { | |
| label: 'Divider weight (px)', | |
| key: 'dividerWeight', | |
| }, | |
| { | |
| label: 'Hover opacity (0-1)', | |
| key: 'hoverOpacity', | |
| }, | |
| ]; | |
| export function Other({config}: OtherProps) { | |
| const {setOtherParams} = useContext(ThemeBuilderContext); | |
| const handleChange = (key: keyof Config['layout']['otherParams'], value: number) => { | |
| setOtherParams({ [key]: value }); | |
| setOtherCssParams(key, value); | |
| }; | |
| return ( | |
| <ConfigSection cols={1} title="Other"> | |
| {configOptions.map(({label, key}) => ( | |
| <NumberInput | |
| key={key} | |
| label={label} | |
| value={config.layout.otherParams[key]} | |
| onChange={(value) => handleChange(key, value)} | |
| /> | |
| ))} | |
| </ConfigSection> | |
| ); | |
| } |
| <NumberInput | ||
| label="Disabled opacity (0-1)" | ||
| value={config.layout.otherParams.disabledOpacity} | ||
| onChange={(value) => { | ||
| setOtherParams({disabledOpacity: value}); | ||
| setOtherCssParams("disabledOpacity", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Divider weight (px)" | ||
| value={config.layout.otherParams.dividerWeight} | ||
| onChange={(value) => { | ||
| setOtherParams({dividerWeight: value}); | ||
| setOtherCssParams("dividerWeight", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Hover opacity (0-1)" | ||
| value={config.layout.otherParams.hoverOpacity} | ||
| onChange={(value) => { | ||
| setOtherParams({hoverOpacity: value}); | ||
| setOtherCssParams("hoverOpacity", value); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Consider refactoring the onChange handlers to reduce duplication.
The onChange handlers for the NumberInput components have similar logic. You can extract this logic to a separate function to reduce duplication and improve maintainability.
Here's an example of how you can refactor the code:
+const handleChange = (key: keyof Config['layout']['otherParams'], value: number) => {
+ setOtherParams({ [key]: value });
+ setOtherCssParams(key, value);
+};
+
<NumberInput
label="Disabled opacity (0-1)"
value={config.layout.otherParams.disabledOpacity}
- onChange={(value) => {
- setOtherParams({disabledOpacity: value});
- setOtherCssParams("disabledOpacity", value);
- }}
+ onChange={(value) => handleChange('disabledOpacity', value)}
/>
<NumberInput
label="Divider weight (px)"
value={config.layout.otherParams.dividerWeight}
- onChange={(value) => {
- setOtherParams({dividerWeight: value});
- setOtherCssParams("dividerWeight", value);
- }}
+ onChange={(value) => handleChange('dividerWeight', value)}
/>
<NumberInput
label="Hover opacity (0-1)"
value={config.layout.otherParams.hoverOpacity}
- onChange={(value) => {
- setOtherParams({hoverOpacity: value});
- setOtherCssParams("hoverOpacity", value);
- }}
+ onChange={(value) => handleChange('hoverOpacity', value)}
/>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.
| <NumberInput | |
| label="Disabled opacity (0-1)" | |
| value={config.layout.otherParams.disabledOpacity} | |
| onChange={(value) => { | |
| setOtherParams({disabledOpacity: value}); | |
| setOtherCssParams("disabledOpacity", value); | |
| }} | |
| /> | |
| <NumberInput | |
| label="Divider weight (px)" | |
| value={config.layout.otherParams.dividerWeight} | |
| onChange={(value) => { | |
| setOtherParams({dividerWeight: value}); | |
| setOtherCssParams("dividerWeight", value); | |
| }} | |
| /> | |
| <NumberInput | |
| label="Hover opacity (0-1)" | |
| value={config.layout.otherParams.hoverOpacity} | |
| onChange={(value) => { | |
| setOtherParams({hoverOpacity: value}); | |
| setOtherCssParams("hoverOpacity", value); | |
| }} | |
| /> | |
| const handleChange = (key: keyof Config['layout']['otherParams'], value: number) => { | |
| setOtherParams({ [key]: value }); | |
| setOtherCssParams(key, value); | |
| }; | |
| <NumberInput | |
| label="Disabled opacity (0-1)" | |
| value={config.layout.otherParams.disabledOpacity} | |
| onChange={(value) => handleChange('disabledOpacity', value)} | |
| /> | |
| <NumberInput | |
| label="Divider weight (px)" | |
| value={config.layout.otherParams.dividerWeight} | |
| onChange={(value) => handleChange('dividerWeight', value)} | |
| /> | |
| <NumberInput | |
| label="Hover opacity (0-1)" | |
| value={config.layout.otherParams.hoverOpacity} | |
| onChange={(value) => handleChange('hoverOpacity', value)} | |
| /> |
| export function setCssColor(colorType: ColorPickerType, value: string, theme: ThemeType) { | ||
| const brandColorsEl = document.getElementById(colorsId); | ||
| const commonColorsEl = document.getElementById(baseColorsId); | ||
| const showcaseEl = document.getElementById(showcaseId); | ||
| const colorWeight = getColorWeight(colorType, theme); | ||
| const themeColor = generateThemeColor(value, theme, colorWeight); | ||
|
|
||
| if (!brandColorsEl || !commonColorsEl || !showcaseEl) return; | ||
|
|
||
| Object.keys(themeColor).forEach((key) => { | ||
| const value = hexToHsl(themeColor[key as keyof ThemeColor]); | ||
|
|
||
| if (key === "DEFAULT") { | ||
| brandColorsEl.style.setProperty(`--nextui-${colorType}`, value); | ||
| commonColorsEl.style.setProperty(`--nextui-${colorType}`, value); | ||
| showcaseEl.style.setProperty(`--nextui-${colorType}`, value); | ||
| } else { | ||
| brandColorsEl.style.setProperty(`--nextui-${colorType}-${key}`, value); | ||
| commonColorsEl.style.setProperty(`--nextui-${colorType}-${key}`, value); | ||
| showcaseEl.style.setProperty(`--nextui-${colorType}-${key}`, value); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling for missing elements.
The function assumes that the elements with IDs colorsId, baseColorsId, and showcaseId exist in the DOM. If any of these elements are not found, the function will return without any error handling or logging.
Consider adding error handling or logging for cases where the elements are not found in the DOM. For example:
if (!brandColorsEl || !commonColorsEl || !showcaseEl) {
console.error("One or more required elements are missing from the DOM.");
return;
}| export function setAllCssVars(config: Config, theme: ThemeType) { | ||
| setCssColor("default", config[theme].brandColor.default, theme); | ||
| setCssColor("primary", config[theme].brandColor.primary, theme); | ||
| setCssColor("secondary", config[theme].brandColor.secondary, theme); | ||
| setCssColor("success", config[theme].brandColor.success, theme); | ||
| setCssColor("warning", config[theme].brandColor.warning, theme); | ||
| setCssColor("danger", config[theme].brandColor.danger, theme); | ||
| setCssColor("foreground", config[theme].baseColor.foreground, theme); | ||
| setCssContentColor(1, config[theme].baseColor.content1); | ||
| setCssContentColor(2, config[theme].baseColor.content2); | ||
| setCssContentColor(3, config[theme].baseColor.content3); | ||
| setCssContentColor(4, config[theme].baseColor.content4); | ||
| setCssBackground(config[theme].baseColor.background); | ||
| setCssFontSize("tiny", config.layout.fontSize.tiny); | ||
| setCssFontSize("small", config.layout.fontSize.small); | ||
| setCssFontSize("medium", config.layout.fontSize.medium); | ||
| setCssFontSize("large", config.layout.fontSize.large); | ||
| setCssLineHeight("tiny", config.layout.lineHeight.tiny); | ||
| setCssLineHeight("small", config.layout.lineHeight.small); | ||
| setCssLineHeight("medium", config.layout.lineHeight.medium); | ||
| setCssLineHeight("large", config.layout.lineHeight.large); | ||
| setCssRadius("small", config.layout.radius.small); | ||
| setCssRadius("medium", config.layout.radius.medium); | ||
| setCssRadius("large", config.layout.radius.large); | ||
| setCssBorderWidth("small", config.layout.borderWidth.small); | ||
| setCssBorderWidth("medium", config.layout.borderWidth.medium); | ||
| setCssBorderWidth("large", config.layout.borderWidth.large); | ||
| setCssOtherColor("divider", config[theme].otherColor.divider); | ||
| setCssOtherColor("focus", config[theme].otherColor.focus); | ||
| setCssOtherColor("overlay", config[theme].otherColor.overlay); | ||
| setOtherCssParams("disabledOpacity", config.layout.otherParams.disabledOpacity); | ||
| setOtherCssParams("dividerWeight", config.layout.otherParams.dividerWeight); | ||
| setOtherCssParams("hoverOpacity", config.layout.otherParams.hoverOpacity); | ||
| } |
There was a problem hiding this comment.
Add error handling for invalid configuration or theme.
The function assumes that the provided config and theme objects have all the required properties and that their values are valid. If the configuration or theme is invalid or missing required properties, the function will set the CSS variables with undefined or invalid values without any error handling or logging.
Consider adding error handling or logging for cases where the configuration or theme might be invalid or missing required properties. For example:
if (!config[theme] || !config[theme].brandColor || !config[theme].baseColor || !config.layout) {
console.error("Invalid configuration or theme provided.");
return;
}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- apps/docs/components/themes/components/configuration/border-widths.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/other.tsx (1 hunks)
- apps/docs/components/themes/components/showcase-component.tsx (1 hunks)
- apps/docs/components/themes/css-vars.ts (1 hunks)
- apps/docs/components/themes/utils/colors.ts (1 hunks)
- apps/docs/components/themes/utils/config.ts (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- apps/docs/components/themes/components/configuration/border-widths.tsx
- apps/docs/components/themes/components/configuration/other.tsx
- apps/docs/components/themes/components/showcase-component.tsx
- apps/docs/components/themes/css-vars.ts
- apps/docs/components/themes/utils/colors.ts
- apps/docs/components/themes/utils/config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (6)
apps/docs/components/themes/utils/shared.ts (2)
1-7: Add error handling for the Clipboard API call.The
copyDatafunction is correctly implemented and uses the appropriate Clipboard API method. However, it does not handle errors that may occur when writing to the clipboard.Consider adding error handling using a try-catch block:
export function copyData(data: string) { + try { navigator.clipboard.writeText(data); + } catch (err) { + console.error('Failed to copy data: ', err); + } }
9-17: Add a comment about the limitation of not handling circular references.The
stringifyDatafunction is correctly implemented and uses the appropriate JSON.stringify method. However, it does not handle circular references in the data, which could lead to an error.Consider adding a comment to document this limitation:
/** * Stringify data * * @param data * @returns + * @throws {TypeError} Will throw an error if the data contains circular references. */ export function stringifyData(data: unknown) { return JSON.stringify(data, null, 2); }apps/docs/components/themes/components/configuration/base-colors.tsx (1)
19-26: Consider refactoring to reduce code duplication.While the current implementation is functional and readable, there is an opportunity to reduce code duplication in the
ColorPickerconfigurations. TheonChange,onClose, andonCopyhandlers for eachColorPickerfollow a similar pattern, with only minor variations based on the specific base color.Consider extracting the common logic into separate functions or creating a higher-order component that encapsulates the shared behavior. This refactoring would make the code more DRY (Don't Repeat Yourself) and easier to maintain.
For example, you could create a
BaseColorPickercomponent that accepts the base color type as a prop and internally handles the specific logic for each color. This would allow you to simplify theBaseColorscomponent and reduce the repetition of similar code blocks.Also applies to: 27-34, 35-42, 43-50, 51-58, 59-66
apps/docs/components/themes/components/configuration/brand-colors.tsx (1)
1-76: Excellent work on theBrandColorscomponent!The component is well-structured, follows a consistent pattern, and effectively encapsulates the logic for configuring brand colors. It provides a user-friendly interface for modifying the colors and seamlessly integrates with the theme configuration using the
useThemeBuilderhook.The use of the
ColorPickercomponent for each brand color type, along with the appropriate callbacks for handling color changes, updates, and copying of configurations, demonstrates a clear separation of concerns and promotes code reusability.Overall, the component is implemented in a clean and maintainable manner, making it easy to understand and extend.
To further improve code reusability and maintainability, consider extracting the repeated
ColorPickerconfiguration into a separate component or function. This would reduce code duplication and make it easier to update the configuration if needed.For example, you could create a
BrandColorPickercomponent that encapsulates the common props and callbacks:interface BrandColorPickerProps { hexColor: string; icon?: React.ReactNode; label: string; type: string; onChange: (hexColor: string) => void; onClose: (hexColor: string) => void; onCopy: (theme: ThemeType) => void; } function BrandColorPicker({hexColor, icon, label, type, onChange, onClose, onCopy}: BrandColorPickerProps) { return ( <ColorPicker hexColor={hexColor} icon={icon} label={label} type={type} onChange={(hexColor) => onChange(hexColor)} onClose={(hexColor) => onClose(hexColor)} onCopy={(theme) => onCopy(theme)} /> ); }Then, you can use the
BrandColorPickercomponent within theBrandColorscomponent:<BrandColorPicker hexColor={config[theme].brandColor.primary} icon={syncIcon} label="Primary" type="primary" onChange={(hexColor) => setCssColor("primary", hexColor, theme)} onClose={(hexColor) => setBrandColor({primary: hexColor}, theme, syncThemes)} onCopy={(theme) => copyBrandColorConfig(config, "primary", theme)} />This refactoring would make the code more concise and easier to maintain.
apps/docs/components/themes/components/color-picker.tsx (2)
1-105: Excellent work on theColorPickercomponent!The component is well-structured, modular, and follows best practices. It effectively utilizes various dependencies to create a comprehensive color picker with a user-friendly interface. The use of hooks for state management and side effects is appropriate and well-implemented.
Just a few minor suggestions:
Consider adding a default value for the
iconprop to improve clarity and avoid potential issues when the prop is not provided.Add comments to explain the purpose of the
useEffecthook. While the code is readable, a brief comment can help other developers quickly understand the intention behind updating the selected color when the popover is opened.Overall, the
ColorPickercomponent is a solid piece of code that enhances the theme customization experience. Great job!
107-142: ThegetColorfunction is a clean and straightforward utility function.It effectively maps the color picker types to their corresponding CSS classes using a
switchstatement. The cases cover all the possible color picker types, and thedefaultcase handles any unmatched types by returningundefined.One minor suggestion:
- Consider adding a return type annotation to the function signature for clarity. While TypeScript can infer the return type, explicitly specifying it can improve readability and maintainability.
-function getColor(type: ColorPickerType) { +function getColor(type: ColorPickerType): string | undefined {Overall, the
getColorfunction is a well-written utility function that complements theColorPickercomponent nicely.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (15)
- apps/docs/components/themes/components/color-picker.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/base-colors.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/border-widths.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/brand-colors.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/font-sizes.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/index.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/line-heights.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/other-colors.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/other.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/radiuses.tsx (1 hunks)
- apps/docs/components/themes/components/copy-button.tsx (1 hunks)
- apps/docs/components/themes/constants.ts (1 hunks)
- apps/docs/components/themes/provider.tsx (1 hunks)
- apps/docs/components/themes/utils/config.ts (1 hunks)
- apps/docs/components/themes/utils/shared.ts (1 hunks)
Additional comments not posted (32)
apps/docs/components/themes/components/configuration/radiuses.tsx (1)
11-42: LGTM!The
Radiusescomponent is well-structured and follows best practices:
- It correctly uses the
useThemeBuilderhook to access thesetRadiusfunction from the theme context.- The
ConfigSectioncomponent is used appropriately to group the radius inputs.- The
NumberInputcomponents are correctly bound to the corresponding radius values in the theme config.- When a radius value is changed, it updates the theme config and sets the corresponding CSS variable.
- The component is correctly typed with the
RadiusesPropsinterface.- It is a functional component, which aligns with React best practices.
- The component is small and focused, which promotes reusability and maintainability.
Great job!
apps/docs/components/themes/components/configuration/border-widths.tsx (1)
1-38: LGTM! The component is well-structured and addresses the past review comments.The
BorderWidthscomponent is implemented correctly and follows a consistent pattern for handling the border width configuration. The extraction of thehandleChangefunction eliminates duplication and improves maintainability, addressing the concerns raised in the past review comments.The component correctly updates both the theme configuration and the CSS variables when the border width values are changed, ensuring that the changes are reflected in the theme.
Great job on the improvements!
apps/docs/components/themes/components/configuration/other.tsx (1)
1-38: LGTM!The
Othercomponent is well-structured, properly typed, and follows best practices for rendering configuration inputs. The use of theuseThemeBuilderhook and thehandleChangefunction ensures a clean and maintainable implementation.The component is reusable and can be easily extended to include additional configuration options if needed.
Great job!
apps/docs/components/themes/components/configuration/line-heights.tsx (1)
1-50: LGTM!The
LineHeightscomponent is well-structured and follows best practices for managing the theme state and updating the CSS variables. The code is clean, readable, and maintainable.apps/docs/components/themes/components/copy-button.tsx (1)
22-61: LGTM!The
CopyButtoncomponent is well-structured and follows best practices. The use of theDropdowncomponent and icons enhances the user experience. TheuseStatehook is used correctly to manage the copied state. The component is properly typed using theCopyButtonPropsinterface. ThehandleCopyfunction is correctly implemented and sets the copied state for 1.5 seconds. The component correctly passes therestprops to theButtoncomponent.apps/docs/components/themes/components/configuration/other-colors.tsx (1)
16-48: LGTM!The
OtherColorscomponent is well-structured and follows a clear pattern for rendering the color pickers. The use of theuseThemeBuilderhook and thesetOtherColorfunction ensures that the theme colors are updated correctly. TheonChange,onClose, andonCopyevent handlers provide a good user experience for modifying and synchronizing the colors. The component is properly typed with theOtherColorsPropsinterface.Overall, the component implementation looks good and does not have any apparent issues or areas for improvement.
apps/docs/components/themes/constants.ts (5)
5-8: LGTM!The color weight constants are defined appropriately.
10-11: LGTM!The regex pattern for floating-point numbers is defined correctly.
13-18: LGTM!The element ID constants are defined appropriately and follow a consistent naming convention.
20-22: LGTM!The local storage key constants are defined appropriately.
24-107: LGTM!The initial light and dark themes, as well as the layout settings, are defined appropriately. The color values are imported from the "@nextui-org/theme" package, ensuring consistency with the NextUI theme. The layout settings cover various aspects of the theme, providing a good starting point for customization.
apps/docs/components/themes/components/configuration/base-colors.tsx (1)
1-69: Well-structured and modular component for configuring base colors!The
BaseColorscomponent is well-organized and effectively utilizes theuseThemeBuilderhook to manage theme updates. The modular design, with the use ofColorPickerandConfigSectioncomponents, promotes code reusability and maintainability.The component provides a user-friendly interface for configuring base colors, with real-time updates to CSS variables and the ability to persist changes to the theme configuration. The
onCopyhandlers add a nice touch, allowing users to easily copy base color configurations across themes.Overall, the component is implemented in a clean and efficient manner, adhering to best practices and promoting a positive user experience.
apps/docs/components/themes/components/configuration/index.tsx (8)
1-26: Imports look good!The imports are well-organized, follow a logical order, and cover all the necessary dependencies for the component. There are no unused imports or missing dependencies.
27-37: Component declaration and state management are properly set up.The
Configurationcomponent is correctly declared as a default export. The state variables and hooks are appropriately named, initialized, and used to manage the selected template, theme configuration, and theme syncing.
39-53:useEffecthook correctly handles theme changes.The
useEffecthook is properly implemented to update the CSS variables and the configuration when the theme changes. It correctly checks for theme changes by comparing the current theme with the previous theme and updates the necessary variables and local storage.
54-60:handleThemeSyncingfunction correctly handles theme syncing.The
handleThemeSyncingfunction is a simple and effective implementation to handle the syncing of the themes. It correctly updates thesyncThemesstate variable based on the provided boolean parameter.
61-67:handleToggleThemefunction correctly toggles the theme.The
handleToggleThemefunction is properly implemented to toggle the theme between light and dark based on the current theme state. It uses thesetThemefunction from theuseThemehook to update the theme correctly.
68-81:handleResetThemefunction correctly resets the theme.The
handleResetThemefunction is properly implemented to reset the theme based on the selected template or default configuration. It correctly updates the configuration and CSS variables using thesetConfiguration,setAllCssVars, andresetConfigfunctions as needed.
83-126: JSX structure and organization are well-designed.The JSX returned by the component is well-structured and organized. It properly renders the necessary sections for theme configuration, including actions, a switch for theme syncing, a template selector, and several theme configuration sections. The use of separate components for each configuration section promotes modularity and readability.
127-127: Component export is correct.The component is properly exported as the default export, allowing it to be easily imported and used in other parts of the application.
apps/docs/components/themes/utils/config.ts (6)
8-36: LGTM!The
generateLayoutConfigfunction correctly maps the layout properties from the inputConfigobject to the outputNextUIPluginConfig["layout"]object. It uses template literals to convert the layout values to the expected units and directly assigns thedisabledOpacity,dividerWeight, andhoverOpacityvalues from the inputConfigobject to the output object.
38-68: LGTM!The
generateThemeColorsConfigfunction correctly generates the theme colors configuration based on the inputConfigobject and the specifiedThemeType. It uses thegenerateThemeColorfunction to generate the color configurations for various colors and directly assigns other colors from the inputConfigobject to the output object. For thecontentcolors, it assigns the color value from the inputConfigobject as theDEFAULTvalue and uses thereadableColorfunction to determine theforegroundcolor based on the contrast.
73-85: LGTM!The
generatePluginConfigfunction correctly generates the complete plugin configuration for the NextUI theme plugin. It combines the theme colors configuration for bothlightanddarkthemes using thegenerateThemeColorsConfigfunction and includes the layout configuration generated by thegenerateLayoutConfigfunction.
87-97: LGTM!The
copyBrandColorConfigfunction correctly copies the brand color configuration for a specific color type and theme to the clipboard. It generates the theme color configuration using thegenerateThemeColorfunction, stringifies the generated color configuration using thestringifyDatafunction, and copies it to the clipboard using thecopyDatafunction.
99-127: LGTM!The
copyBaseColorConfigfunction correctly copies the base color configuration for a specific color type and theme to the clipboard. It uses a switch statement to handle different cases based on thecolorType. For thebackgroundcolor type, it directly copies the color value from the inputConfigobject. For theforegroundcolor type, it generates the theme color configuration using thegenerateThemeColorfunction and copies it to the clipboard. For thecontentcolor types, it copies an object containing theDEFAULTcolor value from the inputConfigobject and theforegroundcolor determined by thereadableColorfunction to the clipboard.
129-135: LGTM!The
copyOtherColorConfigfunction correctly copies the other color configuration for a specific color type and theme to the clipboard. It directly copies the color value for the specifiedcolorTypefrom the inputConfigobject to the clipboard using thecopyDatafunction.apps/docs/components/themes/provider.tsx (6)
1-6: LGTM!The imports are correctly used and there are no issues.
7-46: LGTM!The
ThemeBuilderContextPropsinterface andThemeBuilderContextcreation are correctly implemented. The default values for the context are properly defined.
47-128: LGTM!The
ThemeBuilderProvidercomponent correctly manages the theme configuration state using theuseStateanduseLocalStoragehooks. The update methods (setConfiguration,resetConfig,setBrandColor, andsetBaseColor) properly update the theme configuration based on the provided arguments.
130-165: LGTM!The
setOtherColormethod correctly updates theotherColorproperty of the theme configuration based on the provided arguments. The code is properly structured and follows the same pattern as the other update methods.
167-225: LGTM!The methods to update the layout properties (
setBorderWidth,setLineHeight,setFontSize,setRadius, andsetOtherParams) correctly update the corresponding properties in thelayoutobject of the theme configuration. The code is properly structured and follows a consistent pattern.
227-257: LGTM!The
ThemeBuilderContext.Provideris correctly rendered with the theme configuration and update methods as the context value. TheuseThemeBuildercustom hook is properly defined and provides access to the theme builder context. The error handling in the custom hook ensures that it is used within theThemeBuilderProvidercomponent.
| export function FontSizes({config}: FontSizesProps) { | ||
| const {setFontSize} = useThemeBuilder(); | ||
|
|
||
| return ( | ||
| <ConfigSection title="Font size (rem)"> | ||
| <NumberInput | ||
| label="Tiny" | ||
| value={config.layout.fontSize.tiny} | ||
| onChange={(value) => { | ||
| setFontSize({tiny: value}); | ||
| setCssFontSize("tiny", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Small" | ||
| value={config.layout.fontSize.small} | ||
| onChange={(value) => { | ||
| setFontSize({small: value}); | ||
| setCssFontSize("small", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Medium" | ||
| value={config.layout.fontSize.medium} | ||
| onChange={(value) => { | ||
| setFontSize({medium: value}); | ||
| setCssFontSize("medium", value); | ||
| }} | ||
| /> | ||
| <NumberInput | ||
| label="Large" | ||
| value={config.layout.fontSize.large} | ||
| onChange={(value) => { | ||
| setFontSize({large: value}); | ||
| setCssFontSize("large", value); | ||
| }} | ||
| /> | ||
| </ConfigSection> | ||
| ); | ||
| } |
There was a problem hiding this comment.
LGTM! Consider extracting repeated logic for better maintainability.
The FontSizes component is well-structured and modular, with clear separation of concerns. The use of the useThemeBuilder hook and setCssFontSize function ensures that the theme and CSS variables are updated consistently. The NumberInput components provide a user-friendly way to input font size values, and the component is type-safe with the FontSizesProps interface.
To further improve the component, consider extracting the repeated NumberInput and onChange logic into a separate component or function. This will reduce duplication and make the component more maintainable.
Here's an example of how you could extract the repeated logic:
interface FontSizeInputProps {
label: string;
value: number;
onChange: (value: number) => void;
}
function FontSizeInput({label, value, onChange}: FontSizeInputProps) {
return (
<NumberInput
label={label}
value={value}
onChange={(value) => {
onChange(value);
setCssFontSize(label.toLowerCase(), value);
}}
/>
);
}
export function FontSizes({config}: FontSizesProps) {
const {setFontSize} = useThemeBuilder();
return (
<ConfigSection title="Font size (rem)">
<FontSizeInput
label="Tiny"
value={config.layout.fontSize.tiny}
onChange={(value) => setFontSize({tiny: value})}
/>
<FontSizeInput
label="Small"
value={config.layout.fontSize.small}
onChange={(value) => setFontSize({small: value})}
/>
<FontSizeInput
label="Medium"
value={config.layout.fontSize.medium}
onChange={(value) => setFontSize({medium: value})}
/>
<FontSizeInput
label="Large"
value={config.layout.fontSize.large}
onChange={(value) => setFontSize({large: value})}
/>
</ConfigSection>
);
}This extracts the NumberInput and onChange logic into a separate FontSizeInput component, which takes a label, value, and onChange prop. The FontSizes component then renders a FontSizeInput for each font size option, passing the appropriate props.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/docs/components/navbar.tsx (1)
239-249: Consider placement and feature flag for the Themes NavbarItem.The addition of the Themes NavbarItem effectively supports the introduction of the theme generator feature. However, consider the following points:
Placement: While the current position seems appropriate, discuss with the team if this is the optimal placement for the best user experience.
Feature Flag: Consider implementing a feature flag for this navigation item. This would allow for easier management of the feature across different environments or user groups, especially if the theme generator is not yet ready for all users.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
- apps/docs/components/navbar.tsx (1 hunks)
apps/docs/components/navbar.tsx
Outdated
| </NavbarItem>{" "} | ||
| <NavbarItem> | ||
| <NextLink | ||
| className={navLinkClasses} | ||
| color="foreground" | ||
| data-active={includes(pathname, "themes")} | ||
| href="/themes" | ||
| onClick={() => handlePressNavbarItem("Figma", "/themes")} | ||
| > | ||
| Themes | ||
| </NextLink> |
There was a problem hiding this comment.
Correct the onClick handler argument for the Themes NavbarItem.
The onClick handler for the Themes NavbarItem is using an incorrect first argument. It's currently set to "Figma" instead of "Themes".
Please update the onClick handler as follows:
- onClick={() => handlePressNavbarItem("Figma", "/themes")}
+ onClick={() => handlePressNavbarItem("Themes", "/themes")}This change will ensure that the correct analytics event is tracked when users click on the Themes navigation item.
📝 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.
| </NavbarItem>{" "} | |
| <NavbarItem> | |
| <NextLink | |
| className={navLinkClasses} | |
| color="foreground" | |
| data-active={includes(pathname, "themes")} | |
| href="/themes" | |
| onClick={() => handlePressNavbarItem("Figma", "/themes")} | |
| > | |
| Themes | |
| </NextLink> | |
| </NavbarItem>{" "} | |
| <NavbarItem> | |
| <NextLink | |
| className={navLinkClasses} | |
| color="foreground" | |
| data-active={includes(pathname, "themes")} | |
| href="/themes" | |
| onClick={() => handlePressNavbarItem("Themes", "/themes")} | |
| > | |
| Themes | |
| </NextLink> |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/docs/components/navbar.tsx (1 hunks)
- apps/docs/components/themes/components/configuration/font-sizes.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/components/navbar.tsx
🔇 Additional comments (2)
apps/docs/components/themes/components/configuration/font-sizes.tsx (2)
1-5: LGTM! Imports are well-organized and relevant.The imports are logically structured and include all necessary dependencies for the components in this file. There are no unused imports, which is good for maintaining clean code.
7-9: LGTM! FontSizesProps interface is well-defined.The
FontSizesPropsinterface is concise and correctly defines the expected props for theFontSizescomponent. It uses theConfigtype, which seems to be imported from a types file, ensuring type safety.
| export function FontSizes({config}: FontSizesProps) { | ||
| return ( | ||
| <ConfigSection title="Font size (rem)"> | ||
| <FontSizeInput label="Tiny" type="tiny" value={config.layout.fontSize.tiny} /> | ||
| <FontSizeInput label="Small" type="small" value={config.layout.fontSize.small} /> | ||
| <FontSizeInput label="Medium" type="medium" value={config.layout.fontSize.medium} /> | ||
| <FontSizeInput label="Large" type="large" value={config.layout.fontSize.large} /> | ||
| </ConfigSection> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using an enum or union type for font size keys.
The FontSizes component looks good overall, but we can improve type safety for the type prop passed to FontSizeInput components. Instead of using string literals, consider defining an enum or union type for font size keys.
Here's a suggested improvement:
type FontSizeKey = 'tiny' | 'small' | 'medium' | 'large';
export function FontSizes({config}: FontSizesProps) {
return (
<ConfigSection title="Font size (rem)">
{(['tiny', 'small', 'medium', 'large'] as const).map((size) => (
<FontSizeInput
key={size}
label={size.charAt(0).toUpperCase() + size.slice(1)}
type={size}
value={config.layout.fontSize[size]}
/>
))}
</ConfigSection>
);
}This approach ensures type safety and reduces repetition in the component.
| interface FontSizeInputProps { | ||
| label: string; | ||
| type: keyof ConfigLayout["fontSize"]; | ||
| value: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for the value prop.
The FontSizeInputProps interface is well-defined, but the value prop type could be more specific. Font sizes are typically numbers, so using string might be too broad.
Consider changing the value prop type to number:
interface FontSizeInputProps {
label: string;
type: keyof ConfigLayout["fontSize"];
value: number;
}This change would make the interface more precise and prevent potential type-related issues when working with font sizes.
| function FontSizeInput({label, type, value}: FontSizeInputProps) { | ||
| const {setFontSize} = useThemeBuilder(); | ||
|
|
||
| return ( | ||
| <NumberInput | ||
| label={label} | ||
| value={value} | ||
| onChange={(value) => { | ||
| setFontSize({[type]: value}); | ||
| setCssFontSize(type, value); | ||
| }} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation for font sizes.
The FontSizeInput component looks good overall, but it lacks input validation. Adding validation would prevent users from entering invalid font sizes.
Consider adding a validation function to the onChange handler:
function FontSizeInput({label, type, value}: FontSizeInputProps) {
const {setFontSize} = useThemeBuilder();
const handleChange = (newValue: number) => {
if (newValue > 0 && newValue <= 10) { // Example validation
setFontSize({[type]: newValue});
setCssFontSize(type, newValue);
} else {
// Handle invalid input (e.g., show an error message)
}
};
return (
<NumberInput
label={label}
value={value}
onChange={handleChange}
min={0.1} // Add min prop
max={10} // Add max prop
step={0.1} // Add step prop for finer control
/>
);
}This change adds basic validation and constrains the input range, improving the user experience and preventing potential issues with extreme font sizes.
|
@wingkwong I fixed the ts error and also implemented the feedback from coderabbitai, but the new reviews from coderabbit ai feels a little bit overkill and don't think they improve the quality of code. I don't find that level of abstraction needed for this simple case. Let me know if there is anything else I need to handle. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/docs/components/themes/components/showcase/line-height.tsx (1)
3-14: Consider improving component naming and content for clarity.While the implementation is good, there are a couple of points to consider:
The component name
FontSizeShowcasedoesn't align perfectly with the showcase name "Line height (rem)". Consider renaming it toLineHeightShowcasefor consistency.The text content of each paragraph doesn't provide information about the actual line height values. Consider adding the line height values to each paragraph for better clarity. For example:
<p className="text-tiny">Tiny (line-height: X rem)</p> <p className="text-small">Small (line-height: Y rem)</p> <p className="text-medium">Medium (line-height: Z rem)</p> <p className="text-large">Large (line-height: W rem)</p>Replace X, Y, Z, and W with the actual line height values in rem.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/docs/components/themes/components/showcase/line-height.tsx (1 hunks)
🔇 Additional comments (2)
apps/docs/components/themes/components/showcase/line-height.tsx (2)
1-1: LGTM: Import statement is correct.The import of
ShowcaseComponentis appropriate and necessary for the component's implementation.
3-14: LGTM: Component structure and implementation are solid.The
FontSizeShowcasecomponent is well-structured and follows React best practices. The use of flex layout for spacing elements is appropriate.
|
Thanks. We'll review gradually. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hmm...it's weird why it's complaining about brand color not being defined since...since the types seems to be correct. Will look into it. |
|
I'm unable to see why there is a issue with that. The export interface Config {
light: ConfigColors;
dark: ConfigColors;
layout: ConfigLayout;
}The export type ThemeType = "light" | "dark";And the issue is mentioned in brand-colors.tsx, where it says that
Can it be a Typescript compiler issue or I'm missing something? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apps/docs/components/themes/index.tsx (2)
10-11: LGTM: Component declaration and state initialization look good.The ThemeBuilder component is correctly exported and uses the useState hook appropriately for client-side rendering.
Consider using the
useClientSideEffecthook from@react-hookz/webfor a more declarative approach to handling client-side effects. This can simplify your code and make it more readable:import { useClientSideEffect } from '@react-hookz/web'; export function ThemeBuilder() { const isClient = useClientSideEffect(); // Rest of the component... }
17-27: LGTM: Conditional rendering is well-implemented.The component's return statement effectively uses conditional rendering based on the isClient state. The structure with ThemeBuilderProvider wrapping the main components is a good practice for state management.
Consider adding an aria-label to the loading container div for better accessibility:
- <div className="flex justify-center items-center w-full h-min md:h-[calc(100vh-15rem)]"> + <div className="flex justify-center items-center w-full h-min md:h-[calc(100vh-15rem)]" aria-label="Theme generator loading state">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/docs/components/themes/components/showcase/circular-progress.tsx (1 hunks)
- apps/docs/components/themes/index.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
apps/docs/components/themes/index.tsx (2)
1-8: LGTM: Imports and "use client" directive are correctly implemented.The "use client" directive and import statements are properly structured. The use of NextUI's Progress component aligns with the project's integration of NextUI.
13-15: LGTM: useEffect hook is correctly implemented.The useEffect hook is properly used to update the isClient state after the component mounts. The empty dependency array ensures this effect runs only once, which is the intended behavior for distinguishing between server-side and client-side rendering.
apps/docs/components/themes/components/showcase/circular-progress.tsx (1)
1-4: Imports look good!The necessary components and types are imported correctly. The use of relative paths for local imports and the specific import of CircularProgress from NextUI demonstrate good import practices.
| ); | ||
| } | ||
|
|
||
| const sizes: NextUISize[] = ["sm", "md", "lg"]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving sizes constant to a shared file
The sizes constant is well-defined and typed correctly. However, for better maintainability and reusability, consider moving this constant to a shared file (e.g., constants.ts or theme-constants.ts) if it's used across multiple components in the theme showcase.
// In a new file, e.g., theme-constants.ts
import { NextUISize } from "./types";
export const SHOWCASE_SIZES: NextUISize[] = ["sm", "md", "lg"];
// Then in this file:
import { SHOWCASE_SIZES } from "../../theme-constants";
// ...
<ShowcaseComponent defaultVariant="solid" name="CircularProgress" sizes={SHOWCASE_SIZES}>
{/* ... */}
</ShowcaseComponent>This approach would centralize size definitions and make it easier to maintain consistent sizes across different showcase components.
| export function CircularProgress() { | ||
| return ( | ||
| <ShowcaseComponent defaultVariant="solid" name="CircularProgress" sizes={sizes}> | ||
| <NextUICircularProgress aria-label="default" color="default"> | ||
| Default | ||
| </NextUICircularProgress> | ||
| <NextUICircularProgress aria-label="primary" color="primary"> | ||
| Primary | ||
| </NextUICircularProgress> | ||
| <NextUICircularProgress aria-label="secondary" color="secondary"> | ||
| Secondary | ||
| </NextUICircularProgress> | ||
| <NextUICircularProgress aria-label="success" color="success"> | ||
| Success | ||
| </NextUICircularProgress> | ||
| <NextUICircularProgress aria-label="warning" color="warning"> | ||
| Warning | ||
| </NextUICircularProgress> | ||
| <NextUICircularProgress aria-label="danger" color="danger"> | ||
| Danger | ||
| </NextUICircularProgress> | ||
| </ShowcaseComponent> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well-structured component with room for enhancement
The CircularProgress component is well-organized and demonstrates good practices:
- Proper use of functional components
- Consistent formatting
- Good accessibility with aria-labels
- Comprehensive color variations
To improve reusability, consider accepting props for customization:
- export function CircularProgress() {
+ export function CircularProgress({ customSizes = sizes, customColors = ['default', 'primary', 'secondary', 'success', 'warning', 'danger'] }) {
return (
- <ShowcaseComponent defaultVariant="solid" name="CircularProgress" sizes={sizes}>
+ <ShowcaseComponent defaultVariant="solid" name="CircularProgress" sizes={customSizes}>
- <NextUICircularProgress aria-label="default" color="default">
- Default
- </NextUICircularProgress>
- <NextUICircularProgress aria-label="primary" color="primary">
- Primary
- </NextUICircularProgress>
- <NextUICircularProgress aria-label="secondary" color="secondary">
- Secondary
- </NextUICircularProgress>
- <NextUICircularProgress aria-label="success" color="success">
- Success
- </NextUICircularProgress>
- <NextUICircularProgress aria-label="warning" color="warning">
- Warning
- </NextUICircularProgress>
- <NextUICircularProgress aria-label="danger" color="danger">
- Danger
- </NextUICircularProgress>
+ {customColors.map((color) => (
+ <NextUICircularProgress key={color} aria-label={color} color={color}>
+ {color.charAt(0).toUpperCase() + color.slice(1)}
+ </NextUICircularProgress>
+ ))}
</ShowcaseComponent>
);
}This change would allow users of the component to customize the sizes and colors as needed, making it more flexible for different use cases within the theme showcase.
📝 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 function CircularProgress() { | |
| return ( | |
| <ShowcaseComponent defaultVariant="solid" name="CircularProgress" sizes={sizes}> | |
| <NextUICircularProgress aria-label="default" color="default"> | |
| Default | |
| </NextUICircularProgress> | |
| <NextUICircularProgress aria-label="primary" color="primary"> | |
| Primary | |
| </NextUICircularProgress> | |
| <NextUICircularProgress aria-label="secondary" color="secondary"> | |
| Secondary | |
| </NextUICircularProgress> | |
| <NextUICircularProgress aria-label="success" color="success"> | |
| Success | |
| </NextUICircularProgress> | |
| <NextUICircularProgress aria-label="warning" color="warning"> | |
| Warning | |
| </NextUICircularProgress> | |
| <NextUICircularProgress aria-label="danger" color="danger"> | |
| Danger | |
| </NextUICircularProgress> | |
| </ShowcaseComponent> | |
| ); | |
| } | |
| export function CircularProgress({ customSizes = sizes, customColors = ['default', 'primary', 'secondary', 'success', 'warning', 'danger'] }) { | |
| return ( | |
| <ShowcaseComponent defaultVariant="solid" name="CircularProgress" sizes={customSizes}> | |
| {customColors.map((color) => ( | |
| <NextUICircularProgress key={color} aria-label={color} color={color}> | |
| {color.charAt(0).toUpperCase() + color.slice(1)} | |
| </NextUICircularProgress> | |
| ))} | |
| </ShowcaseComponent> | |
| ); | |
| } |
|
So the problem was related to SSR. The theme builder was rendered on server side and because of this some values were initialized with false. I've added a check to render the theme builder only on client side and while the js is loading, a loader is displayed.
The build succeeded. Let me know if there is anything else I need to fix. Thank you! |
|
Hey @xylish7 thanks for this great contribution 🙏🏻 could you please fix the conflicts? |
|
Sure! Will look into it today 👍🏻 |
|
@jrgarciadev I've fixed the merge conflicts. Only I also run the |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/docs/components/navbar.tsx(1 hunks)apps/docs/package.json(2 hunks)
🔇 Additional comments (2)
apps/docs/package.json (1)
74-74: LGTM! Good choice for the color picker.
The addition of react-colorful is appropriate for implementing the theme generator's color picking functionality. It's a lightweight and well-maintained library.
apps/docs/components/navbar.tsx (1)
240-248: LGTM! The Themes navigation item is well implemented.
The implementation follows the established patterns and correctly integrates with the existing navigation structure. The active state handling, event tracking, and styling are all properly configured.
| @@ -92,6 +93,7 @@ | |||
| "unified": "^11.0.5", | |||
| "unist-util-visit": "^5.0.0", | |||
| "usehooks-ts": "^2.9.1", | |||
| "values.js": "^2.1.1", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using color2k instead of values.js.
The project already includes color2k as a dependency, which provides similar color manipulation functionality and is actively maintained. Consider leveraging color2k instead of adding values.js to:
- Reduce bundle size by reusing existing dependencies
- Maintain consistency in color manipulation across the codebase
- Benefit from active maintenance and updates
Closes #3066
📝 Description
This PR introduces a theme generator to the NextUI documentation. The theme generator is accessible via a new navigation item,
Themes, making it easier for users to create and customize themes.⛳️ Current Behavior
Currently, the NextUI documentation does not include a theme generator, limiting users' ability to easily create and preview custom themes.
🚀 New Behavior
With this PR, a theme generator will be added to the NextUI documentation, allowing users to generate and customize themes directly within the platform.
💣 Breaking Change
No, this PR does not introduce any breaking changes.
📝 Additional Information
There is an issue with the default color in the white theme; it does not perfectly match the tints and shades of the zinc color used by NextUI's default palette. Although a suitable solution hasn't been found yet, this should not be a blocker, as users can manually adjust the colors. Efforts to resolve this issue will continue.
This version is more concise and polished, improving clarity and flow while retaining all important details.
Summary by CodeRabbit
New Features
NavbarItemfor direct navigation to the themes section.FontSizescomponent for configuring font size settings.FontSizeInputcomponents for various font size categories.FontSizeShowcasecomponent to display text elements with different font sizes.CircularProgresscomponent to showcase variations of circular progress indicators.ThemeBuildercomponent to manage theme configurations and loading states.Bug Fixes
Documentation
Chores