docs(themes): adding theme generator #4626
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA comprehensive theme builder and configuration system has been introduced to the documentation application. This new feature allows users to interactively customize UI themes, including color schemes, font settings, border radii, and other design parameters. The implementation spans multiple components and utilities that work together to provide a dynamic theming experience across light and dark modes. Changes
Assessment against linked issues
✨ Finishing Touches
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
|
macci001
force-pushed
the
macci001/eng-882
branch
from
43bec1b to
787a74b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 40
🧹 Nitpick comments (73)
apps/docs/components/themes/css-vars.ts (3)
7-13: Consider caching DOM lookups for performance.
Repeated calls todocument.getElementById()in each function can cause performance overhead. Caching these elements once or passing them as parameters can make the code more efficient and easier to maintain.
47-51: Confirm units for font sizes.
Usingremunits for font sizes is standard, but verify if the rest of the codebase consistently appliesrem. Mixed usage ofpxoremelsewhere might cause inconsistent sizing.
118-157: Minimize repetitive set function calls.
ThesetAllCssVars()function invokes a series of nearly identical setter functions. Consider grouping color or layout properties in loops, or using a more dynamic approach to reduce verbosity.apps/docs/components/themes/templates/heroui.ts (1)
4-8: Consider clarity around config naming.
Exporting the theme config under the nameherouican be confusing if the broader codebase also references the brand or library as "HeroUI." Clarifying the naming or adding documentation might help avoid confusion.apps/docs/app/themes/page.tsx (1)
3-9: Separate layout concerns from content.
Embedding utility classes directly within the component’s div is convenient, but it makes layout changes harder to maintain. Consider abstracting layout styles into a dedicated container component or a CSS/SCSS module.apps/docs/components/icons/index.ts (1)
16-19: Consider organizing exports alphabetically.While the new exports are valid, organizing them alphabetically would improve maintainability and make it easier to locate specific icons.
export * from "./bug"; +export * from "./filters"; export * from "./gaming-console"; export * from "./linear"; export * from "./magic"; export * from "./mirror-left"; export * from "./moon"; export * from "./moon-filled"; export * from "./palette"; +export * from "./palette-round"; +export * from "./scaling"; export * from "./social"; export * from "./star"; export * from "./sun"; export * from "./two-tone"; -export * from "./mirror-left"; -export * from "./palette-round"; -export * from "./filters"; -export * from "./scaling";apps/docs/hooks/use-previous.ts (1)
3-8: Enhance JSDoc documentation.The current documentation could be more descriptive and include an example.
/** - * Holds the previous value of the provided [value] parameter + * A React hook that stores and returns the previous value of a given state. * - * @param value - * @returns + * @param value - The current value to track + * @returns The previous value (undefined on first render) + * + * @example + * ```tsx + * const [count, setCount] = useState(0); + * const previousCount = usePrevious(count); + * // If count is 5, previousCount will be 4 + * ``` */apps/docs/components/themes/templates/index.ts (1)
7-11: Consider adding runtime type validation.While TypeScript provides compile-time type safety, consider adding runtime validation for the template values to ensure they match the expected schema.
import {z} from 'zod'; const templateSchema = z.object({ label: z.string(), name: z.string(), value: z.object({ // Add specific theme configuration validation }) }); export const templates = [ {label: "HeroUI", name: "heroui", value: heroui}, {label: "Coffee", name: "coffee", value: coffee}, {label: "Emerald", name: "emerald", value: emerald}, ].map(template => templateSchema.parse(template));apps/docs/components/themes/utils/shared.ts (1)
21-30: Simplify border width mapping using an object literal.Consider using an object map for better maintainability and easier extension of border types.
+const BORDER_WIDTH_MAP = { + thin: 1, + medium: 2, + default: 4, +} as const; + -export function getBorderWidth(data: Border) { - if (data === "thin") { - return 1; - } - if (data === "medium") { - return 2; - } - - return 4; +export function getBorderWidth(data: Border): number { + return BORDER_WIDTH_MAP[data] ?? BORDER_WIDTH_MAP.default; }apps/docs/components/themes/components/configuration/fonts.tsx (2)
13-16: Consider using an enum or constant for font options.The font options ("inter", "roboto", "outfit", "lora") are hardcoded. Consider defining them as a typed constant or enum to improve maintainability and type safety.
const FONT_OPTIONS = { INTER: "inter", ROBOTO: "roboto", OUTFIT: "outfit", LORA: "lora" } as const; type FontOption = typeof FONT_OPTIONS[keyof typeof FONT_OPTIONS];
13-16: Clarify the purpose of varying border radius classes.The
rounded-tl-*classes follow an increasing pattern but the purpose isn't immediately clear. Consider adding a comment explaining the visual effect or extract to a constant with a descriptive name.apps/docs/components/themes/components/showcase/index.tsx (2)
16-16: Consider making the min-width responsive.The hardcoded
min-w-6xlmight cause horizontal scrolling on smaller screens. Consider using responsive classes or CSS variables for better adaptability.
18-27: Consider implementing lazy loading for showcase components.With multiple UI components being rendered at once, consider using React.lazy() and Suspense for better initial load performance.
const Avatar = React.lazy(() => import("./avatar")); const BreadCrumbs = React.lazy(() => import("./breadcrumbs")); // ... other imports return ( <Suspense fallback={<div>Loading...</div>}> {/* components */} </Suspense> );apps/docs/components/themes/components/configuration/editable-button.tsx (1)
25-25: Use CSS variables for colors instead of hardcoded values.The hardcoded color values (
#0077ff1A,#92c5ff00) should use theme variables for consistency.-"border-primary-500 bg-gradient-to-b from-[#0077ff1A] to-[#92c5ff00]" +"border-primary-500 bg-gradient-to-b from-primary-100/10 to-transparent"apps/docs/components/themes/components/config-section.tsx (1)
16-16: Use theme variables instead of hardcoded colors.Replace hardcoded colors with theme variables for better dark mode support and consistency.
-"text-[#71717A] dark:text-[#A1A1AA]" +"text-default-400 dark:text-default-500"apps/docs/components/themes/components/configuration/default-colors.tsx (1)
15-32: Add loading state handlingThe component should handle loading states when theme configuration is being fetched or updated.
Consider adding loading state handling:
export function DefaultColors({config, theme}: DefaultColorsProp) { - const {setDefaultColor} = useThemeBuilder(); + const {setDefaultColor, isLoading} = useThemeBuilder(); return ( <ConfigSection icon={<PaletteRound className="h-5 w-5" />} id={defaultColorsId} title="Default Colors" + disabled={isLoading} > <ColorPicker hexColor={config[theme].defaultColor.default} type="default" onChange={(hexColor) => setCssColor("default", hexColor, theme)} onClose={(hexColor) => setDefaultColor({default: hexColor}, theme, false)} + disabled={isLoading} /> </ConfigSection> ); }apps/docs/components/themes/components/showcase-component.tsx (1)
26-37: Optimize performance with useMemo and add error boundaryThe component should memoize the style object and handle font loading failures gracefully.
+import {useMemo} from 'react'; +import {ErrorBoundary} from 'react-error-boundary'; export function ShowcaseComponent({children, id, name}: ShowcaseComponentProps) { const {font} = useThemeBuilder(); - const style = getFontStyle(font); + const style = useMemo(() => getFontStyle(font), [font]); return ( + <ErrorBoundary fallback={<div>Failed to load font. Using system font.</div>}> <div className="bg-background text-foreground py-6 p-4 rounded-lg" id={id} style={style}> <span className="text-xl font-medium text-default-800">{name}</span> <Divider className="mt-4 mb-6" /> <div className="flex flex-wrap gap-6 mt-8">{children}</div> </div> + </ErrorBoundary> ); }apps/docs/components/theme-panel.tsx (2)
27-36: Extract theme toggle to a separate componentThe theme toggle logic should be encapsulated in a dedicated component for better maintainability.
+const ThemeToggle = () => { + const {theme, setTheme} = useTheme(); + return ( + <Button + isIconOnly + variant="flat" + aria-label={`Switch to ${theme === 'dark' ? 'light' : 'dark'} theme`} + onPress={() => setTheme(theme === "dark" ? "light" : "dark")} + > + {theme === "dark" ? <SunIcon /> : <MoonIcon />} + </Button> + ); +}; - <Button - isIconOnly - variant="flat" - onPress={() => { - theme === "dark" ? setTheme("light") : setTheme("dark"); - }} - > - {theme == "dark" ? <SunIcon /> : <MoonIcon />} - </Button> + <ThemeToggle />
20-41: Add loading state feedbackUsers should receive visual feedback while the theme is being loaded or changed.
return ( <div className="border p-4 col-span-1 h-[70vh] overflow-scroll"> <h2 className="font-bold text-xl">Theme</h2> <div className="flex gap-x-2 my-2"> + {!isMounted ? ( + <div className="animate-pulse h-9 w-20 bg-gray-200 rounded" /> + ) : ( <Button isIconOnly variant="flat"> <CopyIcon /> </Button> + )} <ThemeToggle /> </div> <div className="my-2"> <h4 className="text-md">Color</h4> </div> </div> );apps/docs/components/themes/components/configuration/radiuses.tsx (1)
13-42: Improve radius preview visualization.Currently, the preview only shows the top-left radius (
rounded-tl-*), which might not give users a complete understanding of how the radius will look when applied. Consider showing the radius on all corners.<EditableButton - className="rounded-tl-none" + className="rounded-none" setValue={setRadiusValue} title="none" value={radiusValue} />Also, consider extracting the repeated button configurations to reduce duplication:
const radiusOptions = [ { title: "none", className: "rounded-none" }, { title: "sm", className: "rounded-sm" }, { title: "md", className: "rounded-md" }, { title: "lg", className: "rounded-lg" }, { title: "full", className: "rounded-full" }, ];apps/docs/components/themes/components/configuration/font-sizes.tsx (1)
35-38: Optimize onChange handler to prevent unnecessary re-renders.The current implementation creates a new function on every render. Consider memoizing the handler or moving it outside the component.
+const handleFontSizeChange = (type: keyof ConfigLayout["fontSize"]) => + (value: string) => { + setFontSize({[type]: value}); + setCssFontSize(type, value); + }; <NumberInput label={label} value={value} - onChange={(value) => { - setFontSize({[type]: value}); - setCssFontSize(type, value); - }} + onChange={handleFontSizeChange(type)} />apps/docs/components/themes/templates/coffee.ts (2)
6-56: Document color usage and relationships.Add JSDoc comments to explain the purpose and relationships between different color variables, making it easier for other developers to understand and maintain the theme.
+/** + * Coffee theme configuration + * + * Color Relationships: + * - Primary: Used for main actions and highlights (#db924b) + * - Secondary: Used for supporting elements (#5a8486) + * - Content scales: Background surfaces from lightest to darkest + */ export const coffee: Config = {
21-21: Standardize color value sources.The theme inconsistently mixes imported colors (from @heroui/theme) with hardcoded values. Consider using the imported colors consistently where applicable.
- overlay: colors.black, + overlay: colors.black[500],Also applies to: 46-46
apps/docs/components/themes/components/configuration/scaling.tsx (1)
8-10: Add validation for scaling values.Consider adding validation to ensure scaling values remain within acceptable range.
export function Scaling() { const {scaling, setScaling} = useThemeBuilder(); + + const handleScaling = (value: number) => { + if (value >= 90 && value <= 110) { + setScaling(value); + } + };apps/docs/components/themes/components/select-template.tsx (1)
37-45: Improve type safety in template mapping.The value prop uses array index which could lead to issues if the templates array order changes.
- value={index} + value={template.name}apps/docs/components/themes/components/configuration/line-heights.tsx (2)
16-23: Reduce code duplication in onChange handlersThe onChange handlers for all four NumberInput components contain identical logic structure. Consider extracting this into a reusable function to improve maintainability.
export function LineHeights({config}: LineHeightsProps) { const {setLineHeight} = useThemeBuilder(); + + const handleLineHeightChange = (size: 'tiny' | 'small' | 'medium' | 'large', value: number) => { + setLineHeight({[size]: value}); + setCssLineHeight(size, value); + }; + return ( <ConfigSection title="Line height (rem)"> <NumberInput label="Tiny" value={config.layout.lineHeight.tiny} - onChange={(value) => { - setLineHeight({tiny: value}); - setCssLineHeight("tiny", value); - }} + onChange={(value) => handleLineHeightChange('tiny', value)} /> // Apply similar changes to other NumberInput componentsAlso applies to: 24-31, 32-39, 40-47
16-47: Add input validation and accessibility improvementsConsider the following enhancements:
- Add min/max constraints for line height values
- Include aria-label attributes for better screen reader support
- Add step size for number inputs
<NumberInput label="Tiny" value={config.layout.lineHeight.tiny} + min={0.5} + max={3} + step={0.1} + aria-label="Set tiny line height" onChange={(value) => handleLineHeightChange('tiny', value)} />apps/docs/components/icons/filters.tsx (2)
3-11: Improve icon accessibility and reusabilityThe icon component could benefit from the following improvements:
- Add aria-label for accessibility
- Make fill color configurable through props
-export const Filters = ({size = 24, width, height, ...props}: IconSvgProps) => ( +interface FiltersProps extends IconSvgProps { + fill?: string; +} + +export const Filters = ({ + size = 24, + width, + height, + fill = "#A1A1AA", + ...props +}: FiltersProps) => ( <svg focusable="false" + aria-label="Filters" + role="img" height={size || height} viewBox="0 0 20 20" width={size || width} xmlns="http://www.w3.org/2000/svg" {...props} >
12-17: Make fill color dynamicUpdate the path to use the configurable fill color.
<path clipRule="evenodd" d="M10.0003 2.29166C7.58408 2.29166 5.62533 4.25041..." - fill="#A1A1AA" + fill={fill} fillRule="evenodd" />apps/docs/components/themes/components/configuration/disable-opacity.tsx (2)
24-51: Reduce duplication and improve type safetyThe component has several areas for improvement:
- Repeated setValue handlers
- Hardcoded opacity values
- Missing type safety
+const OPACITY_VALUES = ['0.2', '0.4', '0.6', '0.8'] as const; +type OpacityValue = typeof OPACITY_VALUES[number]; + export function DisableOpacity({config}: DisableOpacityProps) { const {setOtherParams} = useThemeBuilder(); - const handleChange = (key: keyof Config["layout"]["otherParams"], value: string) => { + const handleChange = (key: keyof Config["layout"]["otherParams"], value: OpacityValue) => { setOtherParams({[key]: value}); setOtherCssParams(key, value); }; return ( <ConfigSection icon={<RadialBlur className="h-5 w-5" />} title="Disable Opacity"> - <ValueButton - currentValue={config.layout.otherParams.disabledOpacity} - setValue={(value) => { - handleChange("disabledOpacity", value); - }} - value={"0.2"} - /> - // ... repeat for other values + {OPACITY_VALUES.map((value) => ( + <ValueButton + key={value} + currentValue={config.layout.otherParams.disabledOpacity} + setValue={(val) => handleChange("disabledOpacity", val as OpacityValue)} + value={value} + /> + ))} </ConfigSection> ); }
23-23: Add tooltip to explain opacity impactConsider adding a tooltip to explain how the disabled opacity affects components.
- <ConfigSection icon={<RadialBlur className="h-5 w-5" />} title="Disable Opacity"> + <ConfigSection + icon={<RadialBlur className="h-5 w-5" />} + title="Disable Opacity" + tooltip="Controls the opacity of disabled components. Higher values make disabled elements more visible." + >apps/docs/components/icons/palette-round.tsx (2)
3-11: Apply same improvements as Filters iconFor consistency with the Filters icon, apply the same improvements:
- Add accessibility attributes
- Make fill color configurable
-export const PaletteRound = ({size = 24, width, height, ...props}: IconSvgProps) => ( +interface PaletteRoundProps extends IconSvgProps { + fill?: string; +} + +export const PaletteRound = ({ + size = 24, + width, + height, + fill = "#A1A1AA", + ...props +}: PaletteRoundProps) => ( <svg focusable="false" + aria-label="Color palette" + role="img" height={size || height} viewBox="0 0 20 20" width={size || width} xmlns="http://www.w3.org/2000/svg" {...props} >
12-17: Make fill color dynamicUpdate the path to use the configurable fill color.
<path clipRule="evenodd" d="M1.04199 4.99999C2.8142 1.04166..." - fill="#A1A1AA" + fill={fill} fillRule="evenodd" />apps/docs/components/themes/components/copy-button.tsx (1)
43-57: Enhance accessibility for dropdown menu itemsAdd aria-label to dropdown items to improve screen reader experience.
<DropdownMenu aria-label="Copy configuration"> <DropdownItem key="light" + aria-label="Copy light theme configuration" startContent={<Icon className="text-lg" icon={SunIcon} />} onPress={() => handleCopy("light")} > Light config </DropdownItem> <DropdownItem key="dark" + aria-label="Copy dark theme configuration" startContent={<Icon className="text-lg" icon={MoonIcon} />} onPress={() => handleCopy("dark")} > Dark config </DropdownItem>apps/docs/components/themes/components/configuration/content-colors.tsx (1)
25-48: Reduce code duplication in ColorPicker implementationsThe ColorPicker components share similar patterns and could be refactored to reduce duplication.
+ const contentColors = [ + { type: "content1", index: 1 }, + { type: "content2", index: 2 }, + { type: "content3", index: 3 }, + { type: "content4", index: 4 }, + ]; + return ( <ConfigSection icon={<PaletteRound className="w-5 h-5" />} id={baseColorsId} title="Content colors" toolTip="content1, content2, content3, content4 colors" > - <ColorPicker - hexColor={config[theme].contentColor.content1} - type="content1" - onChange={(hexColor) => setCssContentColor(1, hexColor)} - onClose={(hexColor) => setConentColor({content1: hexColor}, theme)} - /> - // ... repeated ColorPicker components + {contentColors.map(({ type, index }) => ( + <ColorPicker + key={type} + hexColor={config[theme].contentColor[type]} + type={type} + onChange={(hexColor) => setCssContentColor(index, hexColor)} + onClose={(hexColor) => setContentColor({[type]: hexColor}, theme)} + /> + ))} </ConfigSection> );apps/docs/components/themes/components/showcase/code.tsx (2)
38-59: Refactor scaling switch statement to use a lookup objectThe switch statement contains magic numbers and repetitive patterns. Consider using a lookup object for better maintainability.
- switch (scaling) { - case 90: { - className = "p-2 px-3 text-tiny"; - break; - } - // ... other cases - } + const scalingClasses = { + 90: "p-2 px-3 text-tiny", + 95: "p-2 px-4 text-tiny", + 100: "p-2 px-4 text-medium", + 105: "p-3 px-5 text-medium", + 110: "p-2 px-8 text-medium", + }; + className = scalingClasses[scaling] ?? className;
68-79: Optimize component performance with memoizationThe component could benefit from memoization to prevent unnecessary re-renders.
-export const Code = () => { +import {memo, useMemo} from "react"; + +export const Code = memo(() => { const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; const {radiusValue, scaling} = useThemeBuilder(); + const sections = useMemo(() => + colors.map((color, idx) => ( + <Section key={idx} color={color} radius={radiusValue} scaling={scaling} /> + )), [colors, radiusValue, scaling]); + return ( <ShowcaseComponent name="Code"> - {colors.map((color, idx) => ( - <Section key={idx} color={color} radius={radiusValue} scaling={scaling} /> - ))} + {sections} </ShowcaseComponent> ); -}; +}); + +Code.displayName = "Code";apps/docs/components/themes/components/configuration/layout-colors.tsx (1)
16-52: Reduce code duplication in ColorPicker implementationsSimilar to ContentColors component, the ColorPicker implementations could be refactored to reduce duplication.
+ const layoutColors = [ + { type: "background", key: "background" }, + { type: "foreground", key: "foreground" }, + { type: "focus", key: "focus" }, + { type: "overlay", key: "overlay" }, + ]; + return ( <ConfigSection icon={<PaletteIcon className="w-5 h-5" />} id={otherColorsId} title="Layout colors" toolTip="background, foreground, focus, overlay colors" > - <ColorPicker - hexColor={config[theme].layoutColor.background} - type="background" - onChange={(hexColor) => setCssOtherColor("background", hexColor)} - onClose={(hexColor) => setLayoutColor({background: hexColor}, theme, syncThemes)} - /> - // ... repeated ColorPicker components + {layoutColors.map(({ type, key }) => ( + <ColorPicker + key={type} + hexColor={config[theme].layoutColor[key]} + type={type} + onChange={(hexColor) => setCssOtherColor(key, hexColor)} + onClose={(hexColor) => setLayoutColor({[key]: hexColor}, theme, syncThemes)} + /> + ))} </ConfigSection> );apps/docs/components/themes/components/configuration/base-colors.tsx (1)
27-32: Consider extracting color configuration to reduce repetition.The ColorPicker components follow the same pattern with different color types. This could be refactored to reduce code duplication.
- <ColorPicker - hexColor={config[theme].baseColor.primary} - type="primary" - onChange={(hexColor) => setCssColor("primary", hexColor, theme)} - onClose={(hexColor) => setBaseColor({primary: hexColor}, theme, syncThemes)} - /> - <ColorPicker - hexColor={config[theme].baseColor.secondary} - type="secondary" - onChange={(hexColor) => setCssColor("secondary", hexColor, theme)} - onClose={(hexColor) => setBaseColor({secondary: hexColor}, theme, syncThemes)} - /> - // ... more ColorPicker components + {['primary', 'secondary', 'success', 'warning', 'danger'].map((colorType) => ( + <ColorPicker + key={colorType} + hexColor={config[theme].baseColor[colorType]} + type={colorType} + onChange={(hexColor) => setCssColor(colorType, hexColor, theme)} + onClose={(hexColor) => setBaseColor({[colorType]: hexColor}, theme, syncThemes)} + /> + ))}Also applies to: 33-38, 39-44, 45-50, 51-56
apps/docs/components/themes/utils/config.ts (1)
13-42: Consider memoizing theme color generation.The color generation could be expensive when called frequently. Consider memoizing the results for performance optimization.
+const memoizedThemeColors = new Map(); + function generateThemeColorsConfig(config: Config, theme: ThemeType) { + const key = `${theme}-${JSON.stringify(config[theme])}`; + if (memoizedThemeColors.has(key)) { + return memoizedThemeColors.get(key); + } + const colors = { default: generateThemeColor(config[theme].defaultColor.default, "default", "light"), // ... rest of the colors }; + memoizedThemeColors.set(key, colors); + return colors; }apps/docs/components/themes/components/showcase/popover.tsx (1)
44-67: Consider using a lookup object for scaling classes.The switch statement for scaling classes could be simplified using a lookup object.
- switch (scaling) { - case 90: { - className = "px-1 py-2 text-tiny"; - break; - } - // ... more cases - } + const scalingClasses = { + 90: "px-1 py-2 text-tiny", + 95: "text-tiny px-2 py-3", + 100: "text-small px-2 py-3", + 105: "text-small px-3 py-3", + 110: "text-medium px-3 py-3", + }; + className = scalingClasses[scaling] ?? "text-small px-1";apps/docs/components/themes/components/showcase/switch.tsx (1)
76-77: Consider using map for disabled states.Similar to the Popover component, avoid using cloneElement with static elements and consider using map for the disabled states.
- {cloneElement(<SectionBase />, {color, classNames, isDisabled: false})} - {cloneElement(<SectionBase />, {color, classNames, isDisabled: true})} + {[false, true].map((isDisabled) => ( + <SectionBase + key={isDisabled.toString()} + color={color} + classNames={classNames} + isDisabled={isDisabled} + /> + ))}apps/docs/components/themes/components/showcase/checkbox.tsx (3)
20-20: Replaceanytype with a more specific type definition.Using
anyreduces type safety. Consider defining a proper interface for the classNames object structure.- classNames?: any; + classNames?: { + wrapper: string; + icon: string; + label: string; + };
31-31: Enhance accessibility by using meaningful labels.Using color values as labels may not be user-friendly or accessible. Consider using descriptive labels that convey the purpose of each checkbox.
- {color} + {`${color.charAt(0).toUpperCase()}${color.slice(1)} Option`}
51-92: Optimize the scaling logic by using a lookup object.The switch statement with repeated structures can be simplified using a lookup object, making the code more maintainable and reducing duplication.
- switch (scaling) { - case 90: { - classNames = { - wrapper: "h-5 w-5", - icon: "w-4 h-3", - label: "text-small", - }; - break; - } - // ... other cases - } + const scalingClassNames = { + 90: { + wrapper: "h-5 w-5", + icon: "w-4 h-3", + label: "text-small", + }, + 95: { + wrapper: "h-5 w-5", + icon: "w-4 h-3", + label: "text-medium", + }, + // ... other scales + }; + classNames = scalingClassNames[scaling] ?? classNames;apps/docs/components/themes/types.ts (2)
2-48: Add JSDoc comments to improve type documentation.While the types are well-structured, adding JSDoc comments would improve code documentation and IDE support.
+/** Defines the available color shades from 50 to 900 */ export interface ColorShades { // ... } +/** Defines the available color picker types for theme customization */ export type ColorPickerType = | "background" // ...
89-117: Add value constraints to configuration types.Consider adding string literal types or number ranges for configuration values to ensure valid inputs.
export interface ConfigLayout { fontSize: { - tiny: string; + tiny: `${number}px` | `${number}rem`; // ... similar for other size properties }; otherParams: { - disabledOpacity: string; + disabledOpacity: `${number}%` | number; // ... similar for other numeric parameters }; }apps/docs/components/themes/components/showcase/avatar.tsx (2)
32-32: Consider making the avatar image URL configurable.Hardcoding the avatar image URL reduces component reusability. Consider making it a prop or using a configuration value.
- src="https://i.pravatar.cc/150?u=a04258114e29026708c" + src={props.imageUrl || "https://i.pravatar.cc/150?u=a04258114e29026708c"}
51-52: Improve border width class name generation.The current implementation might not handle all edge cases cleanly. Consider using a more robust approach.
- const borderClassName = border <= 2 ? `ring-${border} ring-offset-2` : `ring ring-offset-2`; + const getBorderClass = (width: number) => { + if (width <= 0) return ''; + if (width <= 2) return `ring-${width} ring-offset-2`; + return 'ring ring-offset-2'; + }; + const borderClassName = getBorderClass(border);apps/docs/components/icons/scaling.tsx (1)
3-21: Enhance icon accessibility with proper ARIA attributes.While the component includes basic ARIA attributes, it could benefit from additional accessibility improvements.
export const Scaling = ({size = 24, width, height, ...props}: IconSvgProps) => ( <svg aria-hidden="true" + aria-label="Scaling icon" fill="none" focusable="false" + xmlns="http://www.w3.org/2000/svg" height={size || height} role="presentation" viewBox="0 0 24 24" width={size || width} {...props} >apps/docs/components/themes/constants.ts (2)
5-11: Enhance regex pattern and add documentation for color weights.
- The float number regex pattern could be improved to:
- Prevent multiple decimal points
- Handle negative numbers
- Limit decimal places
- Consider adding JSDoc comments to explain the purpose and usage of color weights.
// Colors +/** Weight factor used for dark theme color calculations */ export const defaultDarkColorWeight = 20; +/** Weight factor used for light theme color calculations */ export const defaultLightColorWeight = 17.5; +/** Default color weight for theme calculations */ export const colorWeight = 17.5; // Regex +/** Validates float numbers with optional decimal places */ -export const floatNumberPattern = /^\d+(\.\d*)?$/; +export const floatNumberPattern = /^-?\d+(\.\d{0,2})?$/;
26-75: LGTM! Well-structured theme configurations.The light and dark theme configurations are well-organized and maintain symmetry. The content colors follow a logical progression using the zinc color scale.
Consider extracting the common base colors into a shared constant since they're identical in both themes.
+const baseColors = { + primary: colors.blue[500], + secondary: colors.purple[500], + success: colors.green[500], + warning: colors.yellow[500], + danger: colors.red[500], +}; export const initialLightTheme: ConfigColors = { defaultColor: { default: "#d4d4d8", }, - baseColor: { - primary: colors.blue[500], - secondary: colors.purple[500], - success: colors.green[500], - warning: colors.yellow[500], - danger: colors.red[500], - }, + baseColor: baseColors, // ... rest of the config };apps/docs/components/themes/components/showcase/breadcrumbs.tsx (1)
42-96: Refactor scaling logic to reduce repetition and improve maintainability.The switch statement contains magic numbers and repeated patterns for text classes.
+const SCALE_CLASSES = { + 90: { base: "text-[0.7rem]" }, + 95: { base: "text-tiny" }, + 100: { base: "text-small p-0.5" }, + 105: { base: "text-medium p-1" }, + 110: { base: "text-large p-1.5" }, +} as const; const Section = ({/* ... */}) => { // ... - switch (scaling) { - case 90: { - classNames = {base: "text-[0.7rem]"}; - break; - } - case 95: { - classNames = {base: "text-tiny"}; - break; - } - // ... more cases - } + classNames = SCALE_CLASSES[scaling as keyof typeof SCALE_CLASSES] ?? SCALE_CLASSES[100];apps/docs/components/themes/components/showcase/chip.tsx (2)
13-38: Remove unnecessary key prop and make content configurable.
- The
keyprop onHeroUIChipis unnecessary as it's not in an array context.- The chip content is currently limited to the color name.
const SectionBase = ({ color, isDisabled, radius, variant, className, + children, }: { color?: Color; isDisabled?: boolean; radius?: Radius; variant?: Variant; className?: string; + children?: React.ReactNode; }) => { return ( <HeroUIChip - key={radius} className={className} color={color} isDisabled={isDisabled} radius={radius} variant={variant} > - {color} + {children ?? color} </HeroUIChip> ); };
40-109: Optimize performance and simplify className logic.Consider these improvements:
- Memoize variant elements to prevent unnecessary re-renders
- Simplify border class logic using an object map
+import {useMemo} from "react"; +const BORDER_CLASSES = { + thin: "border-small", + medium: "border-medium", + thick: "border-large", +} as const; const Section = ({/* ... */}) => { - let borderClass = "border-medium"; - - if (borderWidthValue === "thin") { - borderClass = "border-small"; - } else if (borderWidthValue === "thick") { - borderClass = "border-large"; - } + const borderClass = BORDER_CLASSES[borderWidthValue]; + const variantElements = useMemo( + () => + variants.map((variant, idx) => + cloneElement(<SectionBase key={idx} />, { + color, + className: clsx( + className, + variant === "bordered" || variant === "faded" ? borderClass : "", + ), + variant, + isDisabled: false, + radius, + }), + ), + [variants, color, className, borderClass, radius] + ); return ( <div key={color} className="flex flex-col gap-y-4"> - {variants.map((variant, idx) => - cloneElement(<SectionBase key={idx} />, { - // ... - }), - )} + {variantElements} {/* ... */} </div> ); };apps/docs/components/themes/components/showcase/input.tsx (1)
39-102: Refactor scaling logic and simplify className handling.The component has similar scaling patterns as other showcase components and complex className handling.
+const SCALE_CLASSES = { + 90: { base: "h-8 w-[300px]", label: "text-tiny" }, + 95: { base: "h-8 w-[320px]", label: "text-tiny" }, + 100: { base: "h-10 w-[340px]", label: "text-small" }, + 105: { base: "h-12 w-[360px]", label: "text-medium" }, + 110: { base: "h-12 w-[380px]", label: "text-medium" }, +} as const; const Section = ({/* ... */}) => { const variants = ["flat", "bordered", "faded", "underlined"]; - let classNames = {base: "h-10 w-[340px]", label: "text-small"}; + const classNames = SCALE_CLASSES[scaling as keyof typeof SCALE_CLASSES] ?? SCALE_CLASSES[100]; const border = getBorderWidth(borderWidthValue); - const borderClassName = `border-${border}`; + const getBorderClass = (variant: string, color: Color) => + variant === "bordered" ? `border-${border} border-${color}` : ""; - switch (scaling) { - case 90: { - classNames = {base: "h-8 w-[300px]", label: "text-tiny"}; - break; - } - // ... remove switch statement - } return ( <div key={color} className="flex flex-col gap-y-4"> {variants.map((variant, idx) => cloneElement(<SectionBase key={idx} />, { color, variant, classNames: { ...classNames, - inputWrapper: clsx(variant === "bordered" ? `${borderClassName} border-${color}` : ""), + inputWrapper: clsx(getBorderClass(variant, color)), }, isDisabled: false, radius, }), )} {/* ... */} </div> ); };apps/docs/components/themes/components/showcase/button.tsx (2)
13-38: Consider adding prop validation for better type safety.While the component works correctly, consider adding runtime validation for the color prop to ensure it matches the allowed values.
+const isValidColor = (color?: Color): boolean => { + const validColors = ["default", "primary", "secondary", "success", "warning", "danger"]; + return color === undefined || validColors.includes(color); +}; const SectionBase = ({ color, radius, isDisabled, variant, className, }: { color?: Color; radius?: Radius; isDisabled?: boolean; variant?: Variant; className?: string; }) => { + if (color && !isValidColor(color)) { + console.warn(`Invalid color prop: ${color}`); + } return ( <HeroUIButton
110-127: Consider memoizing for better performance.Since the colors array is static, it can be moved outside the component. Also, consider using useMemo for the mapped sections if they're expensive to render.
+const BUTTON_COLORS: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; export const Button = () => { - const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; const {radiusValue, scaling, borderWidthValue} = useThemeBuilder(); + const sections = useMemo( + () => + BUTTON_COLORS.map((color, idx) => ( + <Section + key={idx} + borderWidthValue={borderWidthValue} + color={color} + radius={radiusValue} + scaling={scaling} + /> + )), + [borderWidthValue, radiusValue, scaling] + ); return ( <ShowcaseComponent name="Button"> - {colors.map((color, idx) => ( - <Section - key={idx} - borderWidthValue={borderWidthValue} - color={color} - radius={radiusValue} - scaling={scaling} - /> - ))} + {sections} </ShowcaseComponent> ); };apps/docs/components/themes/components/showcase/tabs.tsx (2)
1-11: Enhance type safety for scaling values.Consider defining a union type for the scaling values to prevent invalid numbers.
-import {Border, HeroUIScaling} from "../../types"; +import {Border} from "../../types"; +type HeroUIScaling = 90 | 95 | 100 | 105 | 110;
113-130: Extract shared constants to reduce duplication.Consider moving the colors array to a shared constants file since it's used in both Button and Tabs components.
+// In shared/constants.ts +export const COMPONENT_COLORS = ["default", "primary", "secondary", "success", "warning", "danger"] as const; +export type ComponentColor = typeof COMPONENT_COLORS[number]; -export const TabsComponent = () => { - const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; +export const TabsComponent = () => { + const colors = COMPONENT_COLORS;apps/docs/components/icons/text-square.tsx (1)
1-23: Make icon colors customizable.Consider making the fill colors customizable through props instead of hardcoding them.
-export const TextSquare = ({size = 24, width, height, ...props}: IconSvgProps) => ( +export const TextSquare = ({ + size = 24, + width, + height, + fill = "#A1A1AA", + ...props +}: IconSvgProps & {fill?: string}) => ( <svg focusable="false" height={size || height} viewBox="0 0 20 20" width={size || width} xmlns="http://www.w3.org/2000/svg" {...props} > <path d="M7.29363 4.20835C6.93542..." - fill="#A1A1AA" + fill={fill} /> <path clipRule="evenodd" d="M8.95251 0.0416872C7.02885..." - fill="#A1A1AA" + fill={fill} fillRule="evenodd" /> </svg> );apps/docs/components/icons/mirror-left.tsx (1)
1-25: Standardize icon color handling.
- Make colors customizable through props
- Use consistent colors across paths
-export const MirrorLeft = ({size = 24, width, height, ...props}: IconSvgProps) => ( +export const MirrorLeft = ({ + size = 24, + width, + height, + primaryFill = "#71717A", + secondaryFill = "#A1A1AA", + ...props +}: IconSvgProps & {primaryFill?: string; secondaryFill?: string}) => ( <svg focusable="false" height={size || height} viewBox="0 0 20 20" width={size || width} xmlns="http://www.w3.org/2000/svg" {...props} > <path clipRule="evenodd" d="M11.1174 2.50001C11.1174..." - fill="#71717A" + fill={primaryFill} fillRule="evenodd" /> <path clipRule="evenodd" d="M10 1.04167C10.3452..." - fill="#A1A1AA" + fill={secondaryFill} fillRule="evenodd" /> </svg> );apps/docs/components/themes/utils/colors.ts (3)
18-37: Consider simplifying the reduce operation.The reduce operation could be made more readable using Object.fromEntries and map.
- let shades = colorValues.slice(0, colorValues.length - 1).reduce((acc, shadeValue, index) => { - (acc as any)[index === 0 ? 50 : index * 100] = rgbToHex(shadeValue.rgb); - - return acc; - }, {} as ColorShades); + const shades = Object.fromEntries( + colorValues + .slice(0, -1) + .map((shadeValue, index) => [index === 0 ? 50 : index * 100, rgbToHex(shadeValue.rgb)]) + ) as ColorShades;
42-87: Consider using color2k for color conversion.Since the project already uses color2k (imported for readableColor), consider using it for color conversion to reduce complexity and potential bugs.
-export function hexToHsl(hex: string) { - // Convert hex to RGB first - const [r, g, b] = hexToRgb(hex); - - // Normalize RGB values - const normalizedR = r / 255; - const normalizedG = g / 255; - const normalizedB = b / 255; - - // Find the maximum and minimum values of R, G, B - const max = Math.max(normalizedR, normalizedG, normalizedB); - const min = Math.min(normalizedR, normalizedG, normalizedB); - - // Calculate the lightness - const lightness = (max + min) / 2; - - // If the maximum and minimum are equal, there is no saturation - if (max === min) { - return `${0} ${0}% ${lightness * 100}%`; - } - - // Calculate the saturation - let saturation = 0; - - if (lightness < 0.5) { - saturation = (max - min) / (max + min); - } else { - saturation = (max - min) / (2 - max - min); - } - - // Calculate the hue - let hue; - - if (max === normalizedR) { - hue = (normalizedG - normalizedB) / (max - min); - } else if (max === normalizedG) { - hue = 2 + (normalizedB - normalizedR) / (max - min); - } else { - hue = 4 + (normalizedR - normalizedG) / (max - min); - } - - hue *= 60; - if (hue < 0) hue += 360; - - return `${hue.toFixed(2)} ${(saturation * 100).toFixed(2)}% ${(lightness * 100).toFixed(2)}%`; -} +import {toHsl} from "color2k"; + +export function hexToHsl(hex: string) { + const {h, s, l} = toHsl(hex); + return `${h} ${s}% ${l}%`; +}
103-107: Consider using padStart for cleaner code.The hex padding can be simplified using the padStart method.
-function rgbValueToHex(c: number) { - const hex = c.toString(16); - - return hex.length == 1 ? "0" + hex : hex; -} +function rgbValueToHex(c: number) { + return c.toString(16).padStart(2, "0"); +}apps/docs/components/themes/components/color-picker.tsx (1)
88-123: Consider using a map for color type to class mapping.The switch statement could be replaced with a map for better maintainability and performance.
+const colorTypeToClass = new Map([ + ["primary", "bg-primary text-primary-foreground"], + ["secondary", "bg-secondary text-secondary-foreground"], + ["success", "bg-success text-success-foreground"], + ["warning", "bg-warning text-warning-foreground"], + ["danger", "bg-danger text-danger-foreground"], + ["background", "bg-background text-foreground"], + ["foreground", "bg-foreground text-black"], + ["default", "bg-default"], + ["content1", "bg-content1 text-content1-foreground"], + ["content2", "bg-content2 text-content2-foreground"], + ["content3", "bg-content3 text-content3-foreground"], + ["content4", "bg-content4 text-content4-foreground"], + ["divider", "bg-divider"], + ["focus", "bg-focus"], + ["overlay", "bg-overlay"], +]); + function getColor(type: ColorPickerType) { - switch (type) { - case "primary": - return "bg-primary text-primary-foreground"; - case "secondary": - return "bg-secondary text-secondary-foreground"; - case "success": - return "bg-success text-success-foreground"; - case "warning": - return "bg-warning text-warning-foreground"; - case "danger": - return "bg-danger text-danger-foreground"; - case "background": - return "bg-background text-foreground"; - case "foreground": - return "bg-foreground text-black"; - case "default": - return "bg-default"; - case "content1": - return "bg-content1 text-content1-foreground"; - case "content2": - return "bg-content2 text-content2-foreground"; - case "content3": - return "bg-content3 text-content3-foreground"; - case "content4": - return "bg-content4 text-content4-foreground"; - case "divider": - return "bg-divider"; - case "focus": - return "bg-focus"; - case "overlay": - return "bg-overlay"; - default: - return undefined; - } + return colorTypeToClass.get(type); }apps/docs/components/themes/provider.tsx (1)
83-91: Consider memoizing the reset function.The
resetConfigfunction is recreated on every render but its logic remains constant.+import {useCallback} from "react"; - const resetConfig = (theme: ThemeType, sync: boolean) => { + const resetConfig = useCallback((theme: ThemeType, sync: boolean) => { let newConfig = initialConfig; // ... rest of the function return newConfig; - }; + }, []);apps/docs/components/navbar.tsx (1)
336-346: Consider pluralizing the navigation label.The navigation item's label "Theme" might be better as "Themes" since it leads to a theme generator with multiple theme options.
- Theme + Themesapps/docs/components/themes/components/configuration/index.tsx (3)
65-75: Consider using a debounce for local storage updates.The effect that updates local storage on every config change might cause performance issues with rapid changes.
+import {debounce} from "lodash"; +const debouncedSetLsConfig = debounce((config: Config) => { + setLsConfig(config); +}, 300); useEffect(() => { if (prevTheme !== theme) { setAllCssVars(config, theme); } if (prevTheme === theme) { - setLsConfig(config); + debouncedSetLsConfig(config); } }, [config, theme, prevTheme]);
227-290: Remove unnecessary Fragment.The Fragment wrapper is redundant as it contains only one child element.
- <> <div className="w-full grid grid-cols-4 gap-4 flex-wrap items-center justify-around pt-20"> {/* ... content ... */} </div> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 227-290: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
275-275: Use appropriate icon for font settings.The
RadialBluricon is reused for the font settings, which might not be the most appropriate icon for this context.Consider using a more appropriate icon that represents typography or fonts.
apps/docs/package.json (1)
105-106: Standardize version constraints for new dependencies.The new dependencies use inconsistent version constraints - react-colorful uses
^while values.js uses an exact version.Apply this diff to standardize:
- "react-colorful": "^5.6.1", - "values.js": "^2.1.1" + "react-colorful": "5.6.1", + "values.js": "2.1.1"apps/docs/tailwind.config.js (1)
348-355: Consider a more descriptive utility class name.While
.scrollbar-hideworks, a more descriptive name like.no-scrollbaror.hide-scrollbarwould better align with Tailwind's naming conventions.Apply this diff to improve clarity:
utilities: { - '.scrollbar-hide': { + '.hide-scrollbar': { 'scrollbar-width': 'none', '&::-webkit-scrollbar': { display: 'none', }, } },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
apps/docs/app/themes/page.tsx(1 hunks)apps/docs/components/icons/crop.tsx(1 hunks)apps/docs/components/icons/filters.tsx(1 hunks)apps/docs/components/icons/index.ts(1 hunks)apps/docs/components/icons/mirror-left.tsx(1 hunks)apps/docs/components/icons/palette-round.tsx(1 hunks)apps/docs/components/icons/radial-blur.tsx(1 hunks)apps/docs/components/icons/scaling.tsx(1 hunks)apps/docs/components/icons/text-square.tsx(1 hunks)apps/docs/components/navbar.tsx(1 hunks)apps/docs/components/theme-panel.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/content-colors.tsx(1 hunks)apps/docs/components/themes/components/configuration/default-colors.tsx(1 hunks)apps/docs/components/themes/components/configuration/disable-opacity.tsx(1 hunks)apps/docs/components/themes/components/configuration/editable-button.tsx(1 hunks)apps/docs/components/themes/components/configuration/font-button.tsx(1 hunks)apps/docs/components/themes/components/configuration/font-sizes.tsx(1 hunks)apps/docs/components/themes/components/configuration/fonts.tsx(1 hunks)apps/docs/components/themes/components/configuration/index.tsx(1 hunks)apps/docs/components/themes/components/configuration/layout-colors.tsx(1 hunks)apps/docs/components/themes/components/configuration/line-heights.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/configuration/scaling.tsx(1 hunks)apps/docs/components/themes/components/configuration/swatch.tsx(1 hunks)apps/docs/components/themes/components/configuration/value-button.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/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/code.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/popover.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/heroui.ts(1 hunks)apps/docs/components/themes/templates/index.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/components/themes/utils/shared.ts(1 hunks)apps/docs/hooks/use-previous.ts(1 hunks)apps/docs/package.json(1 hunks)apps/docs/tailwind.config.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/docs/components/themes/components/configuration/index.tsx
[error] 227-290: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (26)
apps/docs/components/themes/css-vars.ts (2)
21-35: Verify consistency when setting default color variations.
Inside theelseblock,defaultColorElis set without appending the-${key}suffix. This might inadvertently override the base property if each color variation gets mapped to the same CSS variable.
71-86: Safeguard edge cases for color readability.
readableColor()might produce undesirable outcomes for unusually bright or dark colors. Consider testing extreme inputs or providing fallbacks for high contrast.apps/docs/hooks/use-previous.ts (1)
9-17: LGTM! Well-implemented custom hook.The implementation follows React best practices and uses generics effectively.
apps/docs/components/themes/index.tsx (1)
9-22: LGTM! Well-implemented hydration safety pattern.The component correctly handles client-side rendering to prevent hydration mismatches.
apps/docs/components/themes/components/configuration/default-colors.tsx (1)
24-29: Add validation and error handling for color valuesThe ColorPicker implementation should validate hex color values and handle potential errors to prevent invalid color states.
Consider adding color validation and error handling:
<ColorPicker hexColor={config[theme].defaultColor.default} type="default" - onChange={(hexColor) => setCssColor("default", hexColor, theme)} + onChange={(hexColor) => { + if (/^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/.test(hexColor)) { + setCssColor("default", hexColor, theme); + } + }} onClose={(hexColor) => setDefaultColor({default: hexColor}, theme, false)} />apps/docs/components/themes/components/configuration/other.tsx (1)
21-25: 🛠️ Refactor suggestionAdd input validation for opacity values.
The opacity inputs should be constrained between 0 and 1. Currently, there are no validation checks to prevent invalid values.
<NumberInput label="Disabled opacity (0-1)" value={config.layout.otherParams.disabledOpacity} + min="0" + max="1" + step="0.1" onChange={(value) => handleChange("disabledOpacity", value)} />✅ Verification successful
Review comment is valid - NumberInput lacks opacity value constraints
The current NumberInput implementation only validates that the input is a valid float number but doesn't constrain the range. The suggested addition of
min,max, andstepprops is necessary because:
- The component allows any float value, which could lead to invalid opacity values
- The underlying Input component from @heroui/react supports these props
- This change would prevent users from entering invalid opacity values outside the 0-1 range
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find NumberInput component definition ast-grep --pattern 'const NumberInput = $_' # Find NumberInput component interface or type ast-grep --pattern 'interface NumberInputProps { $$$ }' # Find other NumberInput usages with opacity rg -A 2 'opacity.*NumberInput|NumberInput.*opacity' --type tsx # Look for existing min/max/step props usage with NumberInput rg 'NumberInput.*min|NumberInput.*max|NumberInput.*step' --type tsxLength of output: 688
Script:
#!/bin/bash # Find other NumberInput usages with opacity rg -t ts -t tsx -A 2 'opacity.*NumberInput|NumberInput.*opacity' # Look for existing min/max/step props usage with NumberInput rg -t ts -t tsx 'NumberInput.*min|NumberInput.*max|NumberInput.*step' # Find the NumberInput component implementation cat apps/docs/components/themes/components/number-input.tsxLength of output: 811
apps/docs/components/themes/components/configuration/font-button.tsx (1)
26-48: Consider adding font loading verification.The component assumes fonts are available but doesn't verify their loading status.
apps/docs/components/themes/components/configuration/base-colors.tsx (1)
10-15: LGTM! Well-structured props interface.The props interface clearly defines the required properties for theme configuration and synchronization.
apps/docs/components/themes/utils/config.ts (1)
7-11: Consider adding validation for disabledOpacity.The layout configuration accepts disabledOpacity without validation. Consider adding bounds checking to ensure valid opacity values.
function generateLayoutConfig(config: Config): HeroUIPluginConfig["layout"] { + const opacity = config.layout.otherParams.disabledOpacity; + if (opacity < 0 || opacity > 1) { + throw new Error("disabledOpacity must be between 0 and 1"); + } return { - disabledOpacity: config.layout.otherParams.disabledOpacity, + disabledOpacity: opacity, }; }✅ Verification successful
Runtime validation for disabledOpacity is necessary
The suggestion to add bounds checking is valid because:
- The value is used directly in CSS variables without validation
- Current implementation only validates number format, not range
- Type system and UI components don't enforce the required 0-1 range
- Documentation explicitly states the value should be between 0 and 1
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for type definitions and interfaces related to Config and HeroUIPluginConfig ast-grep --pattern 'interface Config { $$$ }' ast-grep --pattern 'interface HeroUIPluginConfig { $$$ }' # Search for any existing validation patterns rg "disabledOpacity" -A 2 -B 2 # Look for type definitions with number constraints ast-grep --pattern 'type $_ = number'Length of output: 9595
Script:
#!/bin/bash # Check NumberInput component implementation ast-grep --pattern 'NumberInput' # Search for any validation in the component rg "NumberInput" -A 5 -B 5Length of output: 11969
apps/docs/components/themes/components/showcase/checkbox.tsx (1)
102-113: LGTM! Well-structured component with proper theme integration.The component effectively utilizes the theme builder hook and properly maps through color options.
apps/docs/components/themes/constants.ts (1)
13-21: LGTM! Consistent ID naming convention.The element IDs follow a consistent naming pattern with the 'th-' prefix, making them easily identifiable as theme-related elements.
apps/docs/components/themes/components/showcase/breadcrumbs.tsx (1)
98-109: LGTM! Clean implementation of the showcase component.The component effectively uses the theme builder hook and properly manages the rendering of different breadcrumb variations.
apps/docs/components/themes/components/showcase/chip.tsx (2)
1-11: LGTM! Clear and well-organized type definitions.The type aliases effectively leverage the HeroUI component props, promoting type safety and maintainability.
111-128: LGTM! Well-structured showcase component.The component effectively manages theme values and renders chip variations in a clean and maintainable way.
apps/docs/components/themes/components/showcase/input.tsx (2)
1-13: LGTM! Clean imports and type definitions.The imports are well-organized, and type aliases effectively leverage the HeroUI component props.
104-121: LGTM! Consistent showcase component implementation.The component follows the same clean pattern as other showcase components, effectively using the theme builder hook.
apps/docs/components/themes/components/showcase/button.tsx (1)
1-11: LGTM! Clean imports and type definitions.The code follows TypeScript best practices by using type aliases for better code maintainability.
apps/docs/components/themes/utils/colors.ts (3)
11-13: LGTM! Clean and focused implementation.The function follows the single responsibility principle and effectively converts Values.js color to RGBA string format.
92-98: LGTM! Clean implementation.The function effectively determines color weight based on color type and theme.
112-114: LGTM! Clean implementation.The function effectively converts RGB values to hex format.
apps/docs/components/themes/provider.tsx (2)
338-346: LGTM! Well-implemented custom hook.The custom hook implementation follows React best practices:
- Provides proper type safety
- Includes appropriate error handling for usage outside provider
- Returns the full context object
195-206: 🛠️ Refactor suggestionImprove error handling for clipboard operations.
The
setConentColorfunction is called without error handling for clipboard operations which could fail in certain browsers or contexts.- function handleCopy() { - navigator.clipboard.writeText(JSON.stringify(generatePluginConfig(config), null, 2)); + async function handleCopy() { + try { + await navigator.clipboard.writeText(JSON.stringify(generatePluginConfig(config), null, 2)); + } catch (error) { + console.error('Failed to copy to clipboard:', error); + // Consider showing a user-friendly error message + } + }Likely invalid or redundant comment.
apps/docs/components/themes/components/configuration/actions.tsx (2)
33-59: LGTM! Well-structured UI implementation.The UI implementation is clean and follows good practices:
- Proper use of tooltips for button actions
- Clear visual feedback for copy action
- Consistent icon usage
14-14: 🛠️ Refactor suggestionImprove type definition for onCopy callback.
The
unknownreturn type foronCopyis too permissive and could lead to type-safety issues.- onCopy: () => unknown; + onCopy: () => Record<string, unknown>;Likely invalid or redundant comment.
apps/docs/package.json (1)
105-106: Verify security of new dependencies.Let's verify there are no known vulnerabilities in the new color-related dependencies.
✅ Verification successful
Dependencies verified as secure and well-maintained ✅
Both
react-colorful(5.6.1) andvalues.js(2.1.1) are legitimate, well-maintained packages with:
- No known vulnerabilities
- MIT license compliance
- Active maintenance
- Clear documentation and repositories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in new dependencies echo "Checking react-colorful..." curl -s "https://api.snyk.io/v1/test/npm/react-colorful/5.6.1" -H "Authorization: token $SNYK_TOKEN" echo "Checking values.js..." curl -s "https://api.snyk.io/v1/test/npm/values.js/2.1.1" -H "Authorization: token $SNYK_TOKEN"Length of output: 455
Script:
#!/bin/bash # Create a temporary package.json for npm audit echo '{ "dependencies": { "react-colorful": "5.6.1", "values.js": "2.1.1" } }' > temp_package.json # Run npm audit npm audit --json || true # Check GitHub repos echo "Checking react-colorful GitHub repo..." gh repo view ISvlad/react-colorful --json name,description,updatedAt,stargazerCount,isArchived,defaultBranchRef echo "Checking values.js GitHub repo..." gh repo view noeldelgado/values.js-legacy --json name,description,updatedAt,stargazerCount,isArchived,defaultBranchRefLength of output: 1303
Script:
#!/bin/bash # Get package info from npm registry echo "Checking react-colorful on npm..." curl -s https://registry.npmjs.org/react-colorful | jq '{name, description, maintainers, repository, homepage, license, version, downloads: ."dist-tags", time: .time}' echo "Checking values.js on npm..." curl -s https://registry.npmjs.org/values.js | jq '{name, description, maintainers, repository, homepage, license, version, downloads: ."dist-tags", time: .time}' # Create package.json and generate lockfile echo '{ "dependencies": { "react-colorful": "5.6.1", "values.js": "2.1.1" } }' > temp_package.json # Generate lockfile and run audit npm install --package-lock-only npm audit --jsonLength of output: 6266
apps/docs/tailwind.config.js (1)
348-355: Verify browser compatibility of scrollbar utilities.The implementation handles both Firefox and WebKit browsers, which is good. However, let's verify if there are any other browser-specific considerations.
✅ Verification successful
Browser compatibility for scrollbar utilities is properly implemented ✅
The implementation covers all major browser engines (WebKit/Blink, Gecko, Trident/EdgeHTML) with appropriate vendor-specific properties and includes a way to restore default scrollbar behavior when needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing scrollbar-related utilities and browser compatibility patterns rg -g '*.{js,ts,tsx,css}' -A 2 'scrollbar|::-webkit-scrollbar'Length of output: 7685
apps/docs/components/themes/components/configuration/actions.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
apps/docs/components/themes/components/configuration/actions.tsx (1)
22-24:⚠️ Potential issueImprove clipboard operation handling.
The clipboard operation should be handled asynchronously with proper error handling and user feedback.
- function handleCopyConfig() { + async function handleCopyConfig() { + try { - navigator.clipboard.writeText(JSON.stringify(onCopy(), null, 2)); + await navigator.clipboard.writeText(JSON.stringify(onCopy(), null, 2)); + // Show success feedback to user + } catch (error) { + console.error('Failed to copy configuration:', error); + // Show error feedback to user + } }
🧹 Nitpick comments (23)
apps/docs/components/themes/components/configuration/radiuses.tsx (2)
12-48: Add radiogroup role for improved accessibility.The buttons form a logical group of options but this isn't conveyed to screen readers.
<ConfigSection icon={<CropMinimalistic className="h-5 w-5" />} title="Radius"> + <div role="radiogroup" aria-label="Border radius options"> <EditableButton aria-label="No border radius" className="rounded-tl-none" // ... other props /> // ... other buttons + </div> </ConfigSection>
15-15: Consider enhancing visual representation.The buttons only demonstrate the top-left radius (
rounded-tl-*). Consider showing the radius effect on all corners for better visual feedback.-className="rounded-tl-none" +className="rounded-none" -className="rounded-tl-sm" +className="rounded-sm" -className="rounded-tl-md" +className="rounded-md" -className="rounded-tl-lg" +className="rounded-lg" -className="rounded-tl-full" +className="rounded-full"Also applies to: 22-22, 29-29, 36-36, 43-43
apps/docs/components/themes/components/configuration/scaling.tsx (1)
10-10: Consider moving scale values outside the component.To optimize performance, define
scaleValuesoutside the component to prevent unnecessary array recreation on each render:+const scaleValues = [90, 95, 100, 105, 110]; + export function Scaling() { const {scaling, setScaling} = useThemeBuilder(); - const scaleValues = [90, 95, 100, 105, 110];apps/docs/components/icons/radial-blur.tsx (1)
13-56: Consider making the fill color themeable.The hard-coded fill color (#A1A1AA) might limit theme customization. Consider accepting a color prop or using currentColor to support different themes.
- fill="#A1A1AA" + fill={color || "currentColor"}This change should be applied to all path elements in the SVG.
apps/docs/components/themes/components/configuration/actions.tsx (2)
9-14: Improve type safety for onCopy callback.The
unknownreturn type foronCopyis too permissive. Since the return value is being JSON stringified, it should be constrained to JSON-serializable types.interface ActionsProps { theme: ThemeType; - onCopy: () => unknown; + onCopy: () => Record<string, unknown>; onResetTheme: () => void; onToggleTheme: () => void; }
43-45: Maintain consistency in button styling.The Copy button breaks the pattern of using icons. Consider using a copy icon to maintain visual consistency with other buttons.
<Button isIconOnly color="secondary" size="sm" variant="flat" onClick={handleCopyConfig}> - Copy + <Icon className="text-lg" icon={CopyIcon} /> </Button>You'll need to add this import:
import CopyIcon from "@iconify/icons-solar/copy-linear";apps/docs/components/themes/components/showcase/code.tsx (3)
10-24: Remove unnecessary key prop and optimize performance.Two suggestions for improvement:
- Remove the unnecessary
keyprop fromHeroUICodeas it's not being rendered in a list.- Consider memoizing the component to prevent unnecessary re-renders.
-const SectionBase = ({ +const SectionBase = React.memo(({ color, radius, className, }: { color?: Color; radius?: Radius; className?: string; }) => { return ( - <HeroUICode key={radius} className={className} color={color} radius={radius}> + <HeroUICode className={className} color={color} radius={radius}> npm install @heroui/react </HeroUICode> ); -}; +}); + +SectionBase.displayName = "SectionBase";
26-65: Optimize Section component implementation.Several improvements can be made to enhance maintainability and performance:
- Replace the switch statement with a mapping object
- Memoize the component
- Remove unnecessary div wrapper
+const SCALING_CLASSES: Record<HeroUIScaling, string> = { + 90: "p-2 px-3 text-tiny", + 95: "p-2 px-4 text-tiny", + 100: "p-2 px-4 text-medium", + 105: "p-3 px-5 text-medium", + 110: "p-2 px-8 text-medium", +}; + -const Section = ({ +const Section = React.memo(({ color, radius, scaling, }: { color: Color; radius: Radius; scaling: HeroUIScaling; }) => { - let className = "p-0 px-3 text-tiny"; - - switch (scaling) { - case 90: { - className = "p-2 px-3 text-tiny"; - break; - } - case 95: { - className = "p-2 px-4 text-tiny"; - break; - } - case 100: { - className = "p-2 px-4 text-medium"; - break; - } - case 105: { - className = "p-3 px-5 text-medium"; - break; - } - case 110: { - className = "p-2 px-8 text-medium"; - break; - } - } + const className = SCALING_CLASSES[scaling] ?? "p-0 px-3 text-tiny"; return ( - <div key={color} className="flex flex-col gap-y-4"> - <SectionBase className={className} color={color} radius={radius} /> - </div> + <SectionBase className={className} color={color} radius={radius} /> ); -}; +}); + +Section.displayName = "Section";
67-78: Optimize Code component and fix key prop.Several improvements can be made:
- Move the colors array outside the component to prevent recreation on each render
- Use color value as key instead of array index
- Memoize the component
+const COLORS: Color[] = [ + "default", + "primary", + "secondary", + "success", + "warning", + "danger", +]; + -export const Code = () => { - const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; +export const Code = React.memo(() => { const {radiusValue, scaling} = useThemeBuilder(); return ( <ShowcaseComponent name="Code"> - {colors.map((color, idx) => ( - <Section key={idx} color={color} radius={radiusValue} scaling={scaling} /> + {COLORS.map((color) => ( + <Section key={color} color={color} radius={radiusValue} scaling={scaling} /> ))} </ShowcaseComponent> ); -}; +}); + +Code.displayName = "Code";apps/docs/components/themes/provider.tsx (1)
17-52: Add JSDoc documentation for the interface and its methods.The interface lacks documentation explaining the purpose of each method and the significance of the
syncparameter.Add JSDoc comments to improve maintainability:
+/** + * Context interface for the theme builder functionality. + */ export interface ThemeBuilderContextProps { + /** Current theme configuration */ config: Config; + /** Current radius value */ radiusValue: Radius; // ... add documentation for other properties + /** + * Updates the layout color configuration + * @param newConfig - New layout color configuration + * @param theme - Theme type (light/dark) + * @param sync - Whether to sync changes across both themes + */ setLayoutColor: ( newConfig: Partial<ConfigColors["layoutColor"]>, theme: ThemeType, sync: boolean, ) => void; // ... add documentation for other methods }apps/docs/components/themes/components/showcase/breadcrumbs.tsx (3)
1-1: Consider alternatives tocloneElement.Using
cloneElementcan make the code harder to maintain and debug. Consider refactoring to use component composition or render props pattern instead.- cloneElement(<SectionBase key={idx} />, { + <SectionBase + key={idx} color={color} variant={variant} classNames={{ ...classNames, list: variant === "bordered" ? borderClassName : "", }} isDisabled={disabled[idx]} - }), + />Also applies to: 84-84
58-79: Improve scaling implementation.The scaling implementation can be improved in several ways:
- Magic numbers (90, 95, etc.) should be defined as constants
- Text size classes could be derived programmatically
+const SCALE_CONFIG = { + 90: { size: '[0.7rem]' }, + 95: { size: 'tiny' }, + 100: { size: 'small', padding: '0.5' }, + 105: { size: 'medium', padding: '1' }, + 110: { size: 'large', padding: '1.5' }, +} as const; + +type Scale = keyof typeof SCALE_CONFIG; + switch (scaling) { - case 90: { - classNames = {base: "text-[0.7rem]"}; - break; - } - // ... other cases + case scaling as Scale: { + const config = SCALE_CONFIG[scaling as Scale]; + classNames = { + base: `text-${config.size}${config.padding ? ` p-${config.padding}` : ''}` + }; + break; + } default: { + classNames = {base: "text-small"}; + break; } }
98-101: Extract colors array to constants and add error handling.
- Move the colors array to a constant for better maintainability
- Add error handling for the useThemeBuilder hook
+const BREADCRUMB_COLORS: Color[] = [ + "foreground", + "primary", + "secondary", + "success", + "warning", + "danger", +]; export const BreadCrumbs = () => { - const colors: Color[] = ["foreground", "primary", "secondary", "success", "warning", "danger"]; const {scaling, borderWidthValue} = useThemeBuilder(); + + if (!scaling || !borderWidthValue) { + return null; // or fallback UI + }apps/docs/components/themes/components/showcase/switch.tsx (2)
39-75: Simplify scaling logic using an object lookup.The switch statement could be replaced with a more maintainable object lookup pattern.
Consider this improvement:
- switch (scaling) { - case 90: { - classNames = { - wrapper: "w-8 h-4", - thumb: "w-2 h-2 group-data-[selected=true]:ms-4", - }; - break; - } - // ... other cases - } + const scalingClassNames = { + 90: { + wrapper: "w-8 h-4", + thumb: "w-2 h-2 group-data-[selected=true]:ms-4", + }, + 95: { + wrapper: "w-10 h-6", + thumb: "w-3 h-3", + }, + // ... other mappings + }; + classNames = scalingClassNames[scaling] || classNames;
85-96: Improve component organization and key usage.
- Using array index as key in the map function could cause issues with React's reconciliation.
- The colors array could be moved to constants for better maintainability.
Consider these improvements:
+const SWITCH_COLORS: Color[] = [ + "default", + "primary", + "secondary", + "success", + "warning", + "danger", +]; + export const SwitchComponent = () => { - const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; const {scaling} = useThemeBuilder(); return ( <ShowcaseComponent name="Switch"> - {colors.map((color, idx) => ( - <Section key={idx} color={color} scaling={scaling} /> + {SWITCH_COLORS.map((color) => ( + <Section key={color} color={color} scaling={scaling} /> ))} </ShowcaseComponent> ); };apps/docs/components/themes/components/showcase-component.tsx (1)
26-37: Consider adding aria-label for better accessibility.The showcase component could benefit from better accessibility support.
- <div className="bg-background text-foreground py-6 p-4 rounded-lg" id={id} style={style}> + <div + className="bg-background text-foreground py-6 p-4 rounded-lg" + id={id} + style={style} + aria-label={`${name} showcase section`} + >apps/docs/components/themes/components/configuration/font-button.tsx (1)
17-30: Consider caching font styles for performance.The getFontStyle function creates new objects on each call. Consider memoizing the results.
+const fontStyleCache: Record<FontName, FontStyle> = {}; function getFontStyle(fontName: FontName): FontStyle { + if (fontStyleCache[fontName]) return fontStyleCache[fontName]; + switch (fontName) { case "inter": - return {fontFamily: "'Inter', sans-serif", letterSpacing: "-0.02em"}; + return (fontStyleCache[fontName] = {fontFamily: "'Inter', sans-serif", letterSpacing: "-0.02em"}); // ... rest of the cases } }apps/docs/components/themes/components/showcase/popover.tsx (2)
45-66: Replace switch statement with lookup object for better maintainability.The scaling switch statement can be simplified using a lookup object.
- switch (scaling) { - case 90: { - className = "px-1 py-2 text-tiny"; - break; - } - // ... other cases - } + const scalingClasses = { + 90: "px-1 py-2 text-tiny", + 95: "text-tiny px-2 py-3", + 100: "text-small px-2 py-3", + 105: "text-small px-3 py-3", + 110: "text-medium px-3 py-3" + }; + className = scalingClasses[scaling] || scalingClasses[100];
75-86: Add memoization for color array.The colors array is recreated on each render.
+const POPOVER_COLORS: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; export const PopoverComponent = () => { - const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; const {radiusValue, scaling} = useThemeBuilder(); return ( <ShowcaseComponent name="Popover"> - {colors.map((color, idx) => ( + {POPOVER_COLORS.map((color, idx) => ( <Section key={idx} color={color} radius={radiusValue} scaling={scaling} /> ))} </ShowcaseComponent> ); };apps/docs/components/themes/components/showcase/button.tsx (1)
97-114: Memoize Section components to prevent unnecessary rerenders.The Button component could benefit from memoization to optimize performance.
+import {useMemo} from 'react'; export const Button = () => { const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; const {radiusValue, scaling, borderWidthValue} = useThemeBuilder(); + const sections = useMemo(() => + colors.map((color, idx) => ( + <Section + key={idx} + borderWidthValue={borderWidthValue} + color={color} + radius={radiusValue} + scaling={scaling} + /> + )), [colors, borderWidthValue, radiusValue, scaling]); return ( <ShowcaseComponent name="Button"> - {colors.map((color, idx) => ( - <Section - key={idx} - borderWidthValue={borderWidthValue} - color={color} - radius={radiusValue} - scaling={scaling} - /> - ))} + {sections} </ShowcaseComponent> ); };apps/docs/components/themes/components/color-picker.tsx (3)
24-25: Optimize performance by memoizing color calculations.The
Valuescalculations and color weight computations are being performed on every render. Consider memoizing these values usinguseMemo.- const selectedColorWeight = getColorWeight(type, theme); - const selectedColorValues = new Values(selectedColor).all(selectedColorWeight); + const selectedColorWeight = useMemo(() => getColorWeight(type, theme), [type, theme]); + const selectedColorValues = useMemo( + () => new Values(selectedColor).all(selectedColorWeight), + [selectedColor, selectedColorWeight] + );
62-74: Enhance accessibility for color grid.The color grid items should have proper ARIA labels to improve screen reader support.
<div className="h-6 w-6 rounded" + role="button" + aria-label={`Color shade ${index === 0 ? 50 : index * 100}`} style={{backgroundColor: colorValuesToRgb(colorValue)}} />
89-124: Enhance type safety for color mapping.The
getColorfunction could benefit from exhaustive type checking to ensure all color types are handled.-function getColor(type: ColorPickerType) { +function getColor(type: ColorPickerType): string { switch (type) { // ... existing cases ... default: - return undefined; + const _exhaustiveCheck: never = type; + return ''; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/docs/components/icons/crop.tsx(1 hunks)apps/docs/components/icons/radial-blur.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/border-widths.tsx(1 hunks)apps/docs/components/themes/components/configuration/content-colors.tsx(1 hunks)apps/docs/components/themes/components/configuration/font-button.tsx(1 hunks)apps/docs/components/themes/components/configuration/radiuses.tsx(1 hunks)apps/docs/components/themes/components/configuration/scaling.tsx(1 hunks)apps/docs/components/themes/components/configuration/value-button.tsx(1 hunks)apps/docs/components/themes/components/showcase-component.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/code.tsx(1 hunks)apps/docs/components/themes/components/showcase/popover.tsx(1 hunks)apps/docs/components/themes/components/showcase/switch.tsx(1 hunks)apps/docs/components/themes/provider.tsx(1 hunks)apps/docs/components/themes/utils/colors.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/docs/components/themes/components/configuration/border-widths.tsx
- apps/docs/components/themes/components/config-section.tsx
- apps/docs/components/themes/components/configuration/value-button.tsx
- apps/docs/components/icons/crop.tsx
- apps/docs/components/themes/utils/colors.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (20)
apps/docs/components/themes/components/configuration/radiuses.tsx (3)
1-6: LGTM! Well-organized imports.The imports are logically grouped and all are being used in the component.
8-9: LGTM! Clean component structure.The component follows React best practices with proper hook usage and descriptive naming.
13-19: LGTM! Proper accessibility implementation.The buttons include appropriate ARIA labels for screen readers.
Also applies to: 20-26, 27-33, 34-40, 41-47
apps/docs/components/themes/components/configuration/scaling.tsx (3)
1-6: LGTM! Well-organized imports.The imports are logically grouped and properly spaced for better readability.
8-9: LGTM! Clean component structure.Good use of named export and hook destructuring pattern.
12-24: LGTM! Clean and efficient JSX implementation.The component successfully implements the previous review suggestion to map over scale values, resulting in cleaner and more maintainable code.
apps/docs/components/icons/radial-blur.tsx (2)
1-3: LGTM! Well-structured component declaration.The component follows React best practices with proper typing and default props.
4-12: Accessibility requirements properly implemented.The SVG element includes proper aria-label and focusable attributes as previously requested.
apps/docs/components/themes/components/configuration/actions.tsx (1)
1-7: LGTM! Well-organized imports with performance consideration.Good choice using the offline distribution of icons which helps with performance and reliability.
apps/docs/components/themes/components/showcase/code.tsx (1)
1-8: LGTM! Well-structured imports and type definitions.The imports are well organized and the type definitions are properly derived from CodeProps.
apps/docs/components/themes/provider.tsx (2)
1-16: LGTM! Well-organized imports and type definitions.The imports are properly structured and all necessary types are imported.
337-346: LGTM! Well-implemented custom hook.The hook follows React best practices with proper error handling for usage outside the provider context.
apps/docs/components/themes/components/showcase/breadcrumbs.tsx (1)
15-40: Improve type safety and component flexibility.The previous review suggestions about type safety and component flexibility are still valid.
apps/docs/components/themes/components/showcase/switch.tsx (1)
1-8: LGTM! Well-structured imports and type definitions.The imports are well-organized and the Color type alias improves code maintainability.
apps/docs/components/themes/components/showcase-component.tsx (3)
5-9: LGTM! Well-defined interface with proper type annotations.The ShowcaseComponentProps interface is clear and properly typed.
20-24: LGTM! Clean and type-safe implementation.The getFontStyle function is well-implemented with proper type safety and null checks.
11-18: Consider adding font loading validation.While the font configurations are well-structured, there's no validation to ensure these fonts are actually loaded and available in the browser.
Run this script to check font loading:
apps/docs/components/themes/components/configuration/content-colors.tsx (1)
15-51: 🛠️ Refactor suggestionConsider implementing color scheme validation.
The component should validate color combinations for accessibility and contrast ratios.
+import {validateColorContrast} from '@heroui/utils'; export function ContentColors({config, theme}: BaseColorsProps) { const {setContentColor} = useThemeBuilder(); + + const handleColorChange = (index: number, hexColor: string) => { + const isValid = validateColorContrast(hexColor, config[theme].backgroundColor); + if (!isValid) { + console.warn(`Color combination may have insufficient contrast ratio`); + } + setCssContentColor(index, hexColor); + }; return ( <ConfigSection> <ColorPicker hexColor={config[theme].contentColor.content1} type="content1" - onChange={(hexColor) => setCssContentColor(1, hexColor)} + onChange={(hexColor) => handleColorChange(1, hexColor)} onClose={(hexColor) => setContentColor({content1: hexColor}, theme)} /> {/* Update other ColorPickers similarly */} </ConfigSection> ); }Likely invalid or redundant comment.
apps/docs/components/themes/components/showcase/button.tsx (1)
86-93: 🛠️ Refactor suggestionReplace cloneElement with direct component rendering.
Using cloneElement with a static element is an anti-pattern.
- {cloneElement(<SectionBase />, { - color, - radius, - className, - isDisabled: true, - variant: "solid", - })} + <SectionBase + color={color} + radius={radius} + className={className} + isDisabled={true} + variant="solid" + />Likely invalid or redundant comment.
apps/docs/components/themes/components/color-picker.tsx (1)
1-17: LGTM! Well-structured imports and type definitions.The imports are properly organized and the TypeScript interface is well-defined with clear prop types.
apps/docs/components/themes/components/showcase/breadcrumbs.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/docs/components/themes/components/showcase/breadcrumbs.tsx (2)
15-40: 🛠️ Refactor suggestionImprove type safety and component flexibility further.
While moving the items array out of JSX is good, the component still needs improvements.
Building upon the previous review, apply this diff:
+type BreadcrumbsClassNames = { + base?: string; + list?: string; + item?: string; +}; const SectionBase = ({ color, variant, isDisabled, classNames, + items = ["Home", "Music", "Artist", "Album", "Song"], }: { color?: BreadcrumbsProps["color"]; variant?: BreadcrumbsProps["variant"]; isDisabled?: boolean; - classNames?: any; + classNames?: BreadcrumbsClassNames; + items?: string[]; }) => { - const items = ["Home", "Music", "Artist", "Album", "Song"];
51-52:⚠️ Potential issueFix array length mismatch between variants and disabled arrays.
The variants array has 4 elements (with a duplicate 'solid') while disabled array has 4 elements.
Apply this diff:
- const variants = ["bordered", "light", "solid", "solid"]; - const disabled = [false, false, false, true]; + const variants = ["bordered", "light", "solid"]; + const disabled = [false, false, false];
🧹 Nitpick comments (3)
apps/docs/components/themes/components/showcase/input.tsx (2)
56-77: Consider extracting scaling configuration to a constant.The switch statement contains magic numbers and repeated dimension patterns.
Consider creating a configuration object:
const SCALING_CONFIG = { } as const; // Then use it like: classNames = SCALING_CONFIG[scaling] ?? SCALING_CONFIG[100];
104-121: Improve maintainability and rendering optimization.
- Using array indices as keys may lead to rendering issues
- The colors array should be extracted as a constant
Consider these improvements:
+const INPUT_COLORS: Color[] = [ + "default", + "primary", + "secondary", + "success", + "warning", + "danger", +] as const; export const InputComponent = () => { - const colors: Color[] = ["default", "primary", "secondary", "success", "warning", "danger"]; const {radiusValue, scaling, borderWidthValue} = useThemeBuilder(); return ( <ShowcaseComponent name="Input"> - {colors.map((color, idx) => ( + {INPUT_COLORS.map((color) => ( <Section - key={idx} + key={color} borderWidthValue={borderWidthValue} color={color} radius={radiusValue} scaling={scaling} /> ))} </ShowcaseComponent> ); };apps/docs/components/themes/components/showcase/breadcrumbs.tsx (1)
58-79: Simplify scaling-based className assignment.The switch statement can be replaced with a cleaner object lookup.
Consider this alternative:
- switch (scaling) { - case 90: { - classNames = {base: "text-[0.7rem]"}; - break; - } - case 95: { - classNames = {base: "text-tiny"}; - break; - } - case 100: { - classNames = {base: "text-small p-0.5"}; - break; - } - case 105: { - classNames = {base: "text-medium p-1"}; - break; - } - case 110: { - classNames = {base: "text-large p-1.5"}; - break; - } - } + const scalingClassNames = { + 90: {base: "text-[0.7rem]"}, + 95: {base: "text-tiny"}, + 100: {base: "text-small p-0.5"}, + 105: {base: "text-medium p-1"}, + 110: {base: "text-large p-1.5"}, + }; + classNames = scalingClassNames[scaling] ?? classNames;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/docs/components/themes/components/showcase/breadcrumbs.tsx(1 hunks)apps/docs/components/themes/components/showcase/input.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (4)
apps/docs/components/themes/components/showcase/input.tsx (2)
1-13: LGTM! Clean imports and type definitions.The imports are well-organized and the type aliases are properly derived from InputProps.
14-37: Previous review comment about type safety is still applicable.The suggested improvements for type safety and component flexibility have not been addressed:
- The
classNamesprop is still typed asany- The input label is still hardcoded
apps/docs/components/themes/components/showcase/breadcrumbs.tsx (2)
1-14: LGTM! Clean imports and type definitions.The imports are well-organized and the Color type alias improves code readability.
98-109: LGTM! Clean implementation of the main component.The BreadCrumbs component effectively orchestrates the showcase with proper theme integration.
apps/docs/components/themes/components/showcase/breadcrumbs.tsx
Outdated
Show resolved
Hide resolved
macci001
force-pushed
the
macci001/eng-882
branch
from
cf2c2ca to
4e4087c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/docs/components/themes/components/showcase/breadcrumbs.tsx (2)
13-38: 🛠️ Refactor suggestionImprove type safety by properly typing the classNames prop.
The
classNamesprop is still typed asany, which bypasses TypeScript's type checking benefits.+type BreadcrumbsClassNames = { + base?: string; + list?: string; + item?: string; +}; const SectionBase = ({ color, variant, isDisabled, classNames, }: { color?: BreadcrumbsProps["color"]; variant?: BreadcrumbsProps["variant"]; isDisabled?: boolean; - classNames?: any; + classNames?: BreadcrumbsClassNames;
49-50:⚠️ Potential issueFix array length mismatch between variants and disabled arrays.
The
variantsarray has a duplicate "solid" value anddisabledarray length doesn't match.- const variants = ["bordered", "light", "solid", "solid"]; - const disabled = [false, false, false, true]; + const variants = ["bordered", "light", "solid"]; + const disabled = [false, false, false];
🧹 Nitpick comments (8)
apps/docs/components/themes/components/showcase-component.tsx (1)
25-36: Consider enhancing maintainability and accessibility.While the implementation is solid, consider these improvements:
- Move className strings to constants or a CSS module
- Make the gap spacing configurable via props
- Add aria-label for better accessibility
Here's a suggested implementation:
+const SHOWCASE_STYLES = { + container: "bg-background text-foreground py-6 p-4 rounded-lg", + title: "text-xl font-medium text-default-800", + content: "flex flex-wrap mt-8" +}; +interface ShowcaseComponentProps { children: React.ReactElement | React.ReactElement[]; id?: string; name: string; + gap?: number; } export function ShowcaseComponent({ - children, id, name + children, id, name, gap = 6 }: ShowcaseComponentProps) { const {font} = useThemeBuilder(); const style = getFontStyle(font); return ( <div - className="bg-background text-foreground py-6 p-4 rounded-lg" + className={SHOWCASE_STYLES.container} id={id} style={style} + aria-label={`${name} showcase section`} > - <span className="text-xl font-medium text-default-800">{name}</span> + <span className={SHOWCASE_STYLES.title}>{name}</span> <Divider className="mt-4 mb-6" /> - <div className="flex flex-wrap gap-6 mt-8">{children}</div> + <div className={`${SHOWCASE_STYLES.content} gap-${gap}`}>{children}</div> </div> ); }apps/docs/components/themes/components/showcase/breadcrumbs.tsx (1)
61-82: Simplify the scaling switch statement using an object lookup.The switch statement for scaling can be simplified to improve maintainability.
- switch (scaling) { - case 90: { - classNames = {base: "text-[0.7rem]"}; - break; - } - case 95: { - classNames = {base: "text-tiny"}; - break; - } - case 100: { - classNames = {base: "text-small p-0.5"}; - break; - } - case 105: { - classNames = {base: "text-medium p-1"}; - break; - } - case 110: { - classNames = {base: "text-large p-1.5"}; - break; - } - } + const scalingClassNames = { + 90: {base: "text-[0.7rem]"}, + 95: {base: "text-tiny"}, + 100: {base: "text-small p-0.5"}, + 105: {base: "text-medium p-1"}, + 110: {base: "text-large p-1.5"}, + }; + classNames = scalingClassNames[scaling] || classNames;apps/docs/components/themes/components/configuration/font-button.tsx (2)
18-31: Consider using a constant map for font styles.The switch statement could be replaced with a more maintainable object map.
Consider this refactor:
+const FONT_STYLES: Record<FontName, FontStyle> = { + inter: {fontFamily: "'Inter', sans-serif", letterSpacing: "-0.02em"}, + roboto: {fontFamily: "'Roboto', sans-serif"}, + outfit: {fontFamily: "'Outfit', sans-serif", letterSpacing: "0.05em"}, + lora: {fontFamily: "'Lora', serif"}, +}; + function getFontStyle(fontName: FontName): FontStyle { - switch (fontName) { - case "inter": - return {fontFamily: "'Inter', sans-serif", letterSpacing: "-0.02em"}; - case "roboto": - return {fontFamily: "'Roboto', sans-serif"}; - case "outfit": - return {fontFamily: "'Outfit', sans-serif", letterSpacing: "0.05em"}; - case "lora": - return {fontFamily: "'Lora', serif"}; - default: - return {fontFamily: "'Inter', sans-serif", letterSpacing: "-0.02em"}; - } + return FONT_STYLES[fontName] ?? FONT_STYLES.inter; }
50-52: Remove empty className attribute.The empty className attribute on the div is unnecessary.
Apply this diff:
<div className="relative text-sm text-default-500"> - <div className="">{title}</div> + <div>{title}</div> </div>apps/docs/components/themes/types.ts (3)
2-13: Consider adding hex color validation for color shade values.The
ColorShadesinterface accepts any string for color values. Consider using a more specific type to ensure valid hex color values.+type HexColor = `#${string}`; + export interface ColorShades { - 50: string; + 50: HexColor; // ... apply to all other shades }
46-46: Consider using an enum or const object for scaling values.The magic numbers in
HeroUIScalingwould be more maintainable as named constants.-export type HeroUIScaling = 90 | 95 | 100 | 105 | 110; +export const SCALING = { + XXS: 90, + XS: 95, + BASE: 100, + XL: 105, + XXL: 110, +} as const; +export type HeroUIScaling = typeof SCALING[keyof typeof SCALING];
90-118: Add type safety for numeric configuration values.Consider using more specific types for numeric values in the configuration to ensure valid inputs.
+type CSSSize = `${number}${'px' | 'rem' | 'em' | '%'}`; +type Opacity = `${number}%`; export interface ConfigLayout { fontSize: { - tiny: string; + tiny: CSSSize; // ... apply to all size properties }; otherParams: { - disabledOpacity: string; - hoverOpacity: string; + disabledOpacity: Opacity; + hoverOpacity: Opacity; }; }apps/docs/components/themes/components/showcase/input.tsx (1)
59-80: Replace switch statement with a lookup object for better maintainability.The switch statement for scaling dimensions could be simplified using a lookup object.
- switch (scaling) { - case 90: { - classNames = {base: "h-8 w-[300px]", label: "text-tiny"}; - break; - } - case 95: { - classNames = {base: "h-8 w-[320px]", label: "text-tiny"}; - break; - } - case 100: { - classNames = {base: "h-10 w-[340px]", label: "text-small"}; - break; - } - case 105: { - classNames = {base: "h-12 w-[360px]", label: "text-medium"}; - break; - } - case 110: { - classNames = {base: "h-12 w-[380px]", label: "text-medium"}; - break; - } - } + const scalingMap = { + 90: {base: "h-8 w-[300px]", label: "text-tiny"}, + 95: {base: "h-8 w-[320px]", label: "text-tiny"}, + 100: {base: "h-10 w-[340px]", label: "text-small"}, + 105: {base: "h-12 w-[360px]", label: "text-medium"}, + 110: {base: "h-12 w-[380px]", label: "text-medium"}, + }; + classNames = scalingMap[scaling] ?? scalingMap[100];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/docs/components/themes/components/configuration/font-button.tsx(1 hunks)apps/docs/components/themes/components/showcase-component.tsx(1 hunks)apps/docs/components/themes/components/showcase/breadcrumbs.tsx(1 hunks)apps/docs/components/themes/components/showcase/input.tsx(1 hunks)apps/docs/components/themes/types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (12)
apps/docs/components/themes/components/showcase-component.tsx (2)
1-10: LGTM! Well-structured imports and interface definition.The imports are minimal and the interface is well-typed with clear prop definitions.
12-23: Excellent implementation of type-safe font configurations!The code follows best practices with a maintainable FONT_CONFIGS constant and a concise getFontStyle utility function.
apps/docs/components/themes/components/showcase/breadcrumbs.tsx (2)
1-11: LGTM! Clean imports and type definitions.The imports are well-organized and the type alias improves code readability.
102-113: LGTM! Well-structured showcase component.The component effectively demonstrates various breadcrumb styles and integrates well with the theme builder.
apps/docs/components/themes/components/configuration/font-button.tsx (1)
57-57: LGTM!The export statement is correct.
apps/docs/components/themes/types.ts (2)
50-56: LGTM! Well-structured theme type definitions.The theme types are well-defined and properly extend the color system.
120-128: LGTM! Clear template type definitions.The template types are well-defined and provide good type safety for the theme templates.
apps/docs/components/themes/components/showcase/input.tsx (5)
1-10: LGTM! Clean imports and type definitions.The imports are well-organized and the type aliases are properly derived from InputProps.
12-35: Improve type safety and component flexibility.The previous review comment about type safety remains valid.
- The
classNamesprop is still typed asany- The input label remains hard-coded
Please implement the suggested type improvements from the previous review.
1-125: Well-structured component architecture!The overall component hierarchy is clean, with good separation of concerns and proper prop drilling. The use of the
useThemeBuilderhook for theme customization is appropriate.
114-122: 🛠️ Refactor suggestionUse color value as key instead of array index.
Using array indices as keys may cause issues with React's reconciliation.
- {colors.map((color, idx) => ( - <Section - key={idx} + {colors.map((color) => ( + <Section + key={color} borderWidthValue={borderWidthValue} color={color} radius={radiusValue} scaling={scaling} /> ))}Likely invalid or redundant comment.
84-96: 🛠️ Refactor suggestionUse meaningful keys for list items.
Using array indices as keys may lead to rendering issues when items are reordered or removed.
- {variants.map((variant, idx) => ( - <SectionBase - key={idx} + {variants.map((variant) => ( + <SectionBase + key={`${color}-${variant}`} classNames={{ ...classNames, inputWrapper: clsx(clsx(variant === "bordered" && borderClass)), }} color={color} isDisabled={false} radius={radius} variant={variant as Variant} /> ))}Likely invalid or redundant comment.
Closes #3066
📝 Description
PR adds the theme generator
base pr: #3707 (credits: @xylish7)
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Release Notes
New Features
Crop,Filters,MirrorLeft,PaletteRound,RadialBlur,Scaling, andTextSquare.ColorPickercomponent for color selection.ConfigSectioncomponent for organizing configuration settings.ThemesPagecomponent for theme management.Components
Actions,BaseColors,ContentColors,DefaultColors,DisableOpacity,Fonts,Radiuses,Scaling, andSwatchcomponents for theme customization.Styling
Dependencies
react-colorfulandvalues.jsfor advanced color manipulation.