[WEB-4025] Add useTheming hook, reduce generated TW classes #510
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jamiehenson
force-pushed
the
reduce-tw-class-generation
branch
2 times, most recently
from
f943999 to
19c0991
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: 9
🧹 Outside diff range and nitpick comments (10)
src/core/hooks/useTheming.tsx (1)
10-13: Consider differentiating default values forbaseThemeandtheme.While providing default values is good practice, having the same default value ("dark") for both
baseThemeandthememight lead to unexpected behavior if only one is provided. Consider setting different defaults or documenting the intended behavior clearly.src/core/Tooltip.tsx (1)
190-193: Approve changes with a suggestion for improved readability.The updates to the tooltip's
classNamecorrectly implement the new theming approach and maintain the existing functionality. However, the line is quite long and could be split for better readability.Consider refactoring the
classNameassignment for improved readability:className={` ${themeColor("bg-neutral-1000")} ${themeColor("text-neutral-200")} ui-text-p3 font-medium p-16 ${interactive ? "" : "pointer-events-none"} rounded-lg absolute ${tooltipProps?.className ?? ""} ${fadeOut ? "animate-[tooltipExit_0.25s_ease-in-out]" : "animate-[tooltipEntry_0.25s_ease-in-out]"} `}This format improves readability without changing the functionality.
src/core/styles/colors/utils.ts (1)
1-10: Ensure all imported color arrays are necessary.The imports of various color arrays (
blueColors,greenColors,neutralColors,orangeColors,pinkColors,violetColors,yellowColors) from./typesshould be verified to confirm they are all required. Unused imports may lead to unnecessary code bloat.scripts/compute-colors.ts (1)
64-66: Update the Console Message to Reflect Single Output FileThe console message states that "Tailwind theme classes have been computed and written to JSON files," but only one file (
computed-colors.json) is being written. Update the message to accurately reflect this.Apply this diff to correct the message:
- `🎨 Tailwind theme classes have been computed and written to JSON files.`, + `🎨 Tailwind theme classes have been computed and written to computed-colors.json.`,src/core/ProductTile.tsx (1)
22-25: Confirm the necessity of specifyingbaseTheme: "dark".If
"dark"is the defaultbaseThemeinuseTheming, you might not need to specify it explicitly. Consider omittingbaseThemeif it's unnecessary.src/core/Pricing/PricingCards.tsx (5)
95-95: Simplify complexclassNamefor better readabilityThe
classNameon line 95 is quite complex due to multiple inline conditionals and template literals. Consider extracting parts of this string into variables or using a utility likeclsxorclassnamesto enhance readability and maintainability.For example, you could refactor as:
const borderClass = themeColor(borderClasses(border?.color)?.border ?? "border-neutral-1100"); const styleClass = border?.style ?? ""; const responsiveClass = delimiter ? "@[520px]:flex-row @[920px]:flex-col" : ""; className={`relative border ${borderClass} ${styleClass} flex-1 px-24 py-32 flex flex-col gap-24 rounded-2xl group ${responsiveClass} min-w-[272px] backdrop-blur`}
152-152: SimplifyadditionalCSSinFeaturedLinkcomponentThe
additionalCSSprop on line 152 is quite extensive. Consider breaking it into variables or using a utility function to enhance readability.For example:
const ctaClasses = `text-center ui-btn ${themeColor("bg-neutral-000")} ${themeColor("text-neutral-1300")} hover:text-neutral-000 px-24 !py-12 ${cta.className ?? ""} cursor-pointer`; <FeaturedLink additionalCSS={ctaClasses} url={cta.url} onClick={cta.onClick} disabled={cta.disabled} > {cta.text} </FeaturedLink>
181-181: Review complexclassNamefor list itemsThe
classNamefor list items on line 181 is complex due to multiple conditional expressions. Refactoring this into variables may improve readability.Example:
const paddingClass = index === 0 ? "py-8" : "py-4"; const backgroundClass = index > 0 && index % 2 === 0 ? `${themeColor("bg-blue-900")} rounded-md` : ""; className={`flex justify-between gap-16 px-8 -mx-8 ${paddingClass} ${backgroundClass}`}
218-220: Manage longadditionalCSSstringsThe
additionalCSSprop on lines 218-220 is quite lengthy. Consider refactoring it into variables for clarity.Example:
const featuredLinkClasses = `absolute sm:-translate-x-120 sm:opacity-0 sm:group-hover:translate-x-24 duration-300 delay-0 sm:group-hover:delay-100 sm:group-hover:opacity-100 transition-[transform,opacity] font-medium ui-text-p3 ${themeColor("text-neutral-500")} hover:${themeColor("text-neutral-000")} cursor-pointer`; <FeaturedLink url={cta.url} additionalCSS={featuredLinkClasses} onClick={cta.onClick} iconColor={themeColor(listItemColors?.foreground ?? "text-white")} > {cta.text} </FeaturedLink>
227-227: Simplify complexclassNamefor transition effectsThe
classNameon line 227 includes multiple transition classes. Extracting these into variables can enhance readability.Example:
const transitionClasses = "absolute sm:translate-x-24 sm:opacity-100 sm:group-hover:translate-x-120 sm:group-hover:opacity-0 duration-200 delay-100 sm:group-hover:delay-0 transition-[transform,opacity]"; const textClasses = `leading-6 tracking-widen-0.15 font-light text-p3 ${themeColor("text-neutral-800")}`; className={`${transitionClasses} ${textClasses}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- .gitignore (1 hunks)
- package.json (1 hunks)
- scripts/compute-colors.ts (1 hunks)
- src/core/Icon/EncapsulatedIcon.tsx (2 hunks)
- src/core/Pricing/PricingCards.tsx (11 hunks)
- src/core/ProductTile.tsx (4 hunks)
- src/core/Tooltip.tsx (4 hunks)
- src/core/hooks/useTheming.tsx (1 hunks)
- src/core/styles/colors/Colors.stories.tsx (2 hunks)
- src/core/styles/colors/utils.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- package.json
🧰 Additional context used
🔇 Additional comments (30)
src/core/hooks/useTheming.tsx (3)
1-8: LGTM: Imports and type definition are well-structured.The imports are appropriate for the hook's functionality, and the
UseThemingPropstype is well-defined with optional properties, allowing flexibility in usage.
14-18: LGTM:themeColorfunction is well-implemented.The
themeColorfunction is efficiently memoized usinguseCallback. The logic for determining the color is concise and clear, and the dependency array is correctly specified.
1-25: Overall, theuseTheminghook is well-implemented.The hook is well-structured, follows React best practices, and provides a clear API for theme-based color management. The use of TypeScript for type definitions and
useCallbackfor performance optimization are commendable. Consider the suggestions for default values and return structure to enhance future maintainability and extensibility.src/core/Icon/EncapsulatedIcon.tsx (4)
3-4: LGTM: Import changes align with PR objectivesThe introduction of the
useTheminghook and the removal ofColorClassanddetermineThemeColorimports are consistent with the PR's goal of reducing generated Tailwind classes and improving theming. These changes suggest a more centralized approach to managing theme-related functionality.
21-24: LGTM: Proper implementation of useTheming hookThe
useTheminghook is correctly implemented with the appropriate parameters. The destructuredthemeColorfunction effectively replaces the previousdetermineThemeColorutility, aligning with the PR's objective of introducing the new hook. This change should contribute to reducing generated Tailwind classes and improving theming consistency.
32-32: LGTM: Consistent application of new theming approach in JSXThe changes in the JSX return statement correctly implement the new theming approach using the
themeColorfunction. The existing functionality, including conditional class application based on thethemeprop, is preserved. These modifications align with the PR's goal of reducing generated Tailwind classes while maintaining the component's behavior.Also applies to: 36-36
Line range hint
1-46: Overall assessment: Successful implementation of useTheming hookThe changes in this file successfully implement the new
useTheminghook, replacing the previousdetermineThemeColorutility. This modification aligns well with the PR objectives of introducing theuseTheminghook and reducing generated Tailwind classes. The new approach should lead to improved theming consistency across the application while maintaining the existing functionality of theEncapsulatedIconcomponent.Key improvements:
- Centralized theming logic through the
useTheminghook.- Reduced reliance on generated Tailwind classes.
- Maintained component functionality and conditional theming.
These changes contribute positively to the codebase's maintainability and consistency.
src/core/Tooltip.tsx (4)
14-14: LGTM: New hook import added correctly.The import statement for the
useTheminghook is properly added and follows the correct relative path convention.
169-169: LGTM: Icon color updated to use new theming approach.The Icon component's color prop has been correctly updated to use the
themeColorfunction from the new theming hook. This change maintains consistency with the new theming strategy.
Line range hint
1-203: Overall assessment: Changes successfully implement new theming approach.The modifications to the Tooltip component effectively integrate the new
useTheminghook and consistently apply the updated theming strategy throughout the component. The core functionality of the Tooltip remains intact, with improvements to the theming system.A few minor suggestions have been made for verification and readability, but overall, the changes appear to be well-implemented and maintain the component's intended behavior.
41-44: Verify the baseTheme setting in useTheming hook.The implementation of the
useTheminghook looks good and correctly replaces the previousdetermineThemeColorutility. However, please confirm that settingbaseTheme: "light"is the intended behavior for this component.To ensure consistency across the codebase, let's check how
baseThemeis set in other components:src/core/styles/colors/utils.ts (2)
25-36: Consider edge cases for color variants outside the defined ranges.The function may not handle cases where the
classNameincludes colors or variants not defined in theextentsobject, potentially leading toNaNor incorrect calculations.Please ensure that input validation is added to handle unexpected
classNamevalues gracefully.
25-36: Verify that theextentsobject covers all possible colors.The
invertTailwindClassVariantfunction relies on theextentsobject to determine the flipping logic. It's crucial to ensure that all colors used in the application are included inextents. Missing colors could lead toundefinedvalues and runtime errors.Run the following script to check for any color classes used in the codebase that are not covered by
extents:Expected Result:
- The script should output an empty list, indicating that all used colors are covered.
- If any colors are listed, they are missing from
extentsand should be added.src/core/ProductTile.tsx (1)
46-46: 🛠️ Refactor suggestionAvoid using negative margins with arbitrary values.
The negative margin
mt-[-3px]might lead to maintenance issues and inconsistencies. Consider using standard spacing utilities provided by Tailwind CSS or adjusting the layout to eliminate the need for negative margins.Apply this diff to remove the negative margin:
- className={`ui-text-p2 ${themeColor("text-neutral-000")} font-bold ${unavailable ? "" : "mt-[-3px]"}`} + className={`ui-text-p2 ${themeColor("text-neutral-000")} font-bold`}Ensure that removing the negative margin does not adversely affect the visual layout. If spacing adjustments are needed, consider using predefined margin classes from Tailwind CSS.
src/core/Pricing/PricingCards.tsx (13)
9-9: ImportuseTheminghook correctlyThe
useTheminghook is imported properly, ensuring theming capabilities are available in the component.
48-51: Verify hardcoding ofbaseThemeto"dark"In the
useTheminghook,baseThemeis set to"dark". If the component is intended to support multiple base themes, consider makingbaseThemedynamic or deriving it from props for greater flexibility.
62-62: Proper use ofthemeColorfor additionalCSSApplying
themeColor("text-neutral-500")to theadditionalCSSprop of theIconcomponent ensures consistent theming.
105-105: Ensure background theming withthemeColorThe use of
themeColor("bg-neutral-1300")correctly applies theme-based background coloring.
113-113: Consistent theming for title textApplying
themeColorto the title ensures that text color adheres to the selected theme.
127-127: Themed styling for description textUsing
themeColor(description.color ?? "text-neutral-000")appropriately applies theming to the description text.
139-139: Proper use ofthemeColorfor price amountThe price amount text uses
themeColor("text-neutral-000"), maintaining consistency in theming.
143-145: Correct theming for price contentThe
ui-text-p3class combined withthemeColor("text-neutral-000")ensures the price content is styled according to the theme.
162-164: Themed styling for horizontal ruleThe
<hr>element correctly usesthemeColor("border-neutral-800")for theming the border color.
172-172: Appropriate theming for section titlesApplying
themeColor("text-neutral-500")to section titles ensures they align with the overall theme.
186-186: Consistent text styling in list itemsThe usage of
ui-text-p3and conditional font weights aligns with design standards.
197-200: EnsurelistItemColorsare defined before useWhen using
listItemColorsto setcolorandsecondaryColor, ensure thatlistItemColorsis defined to prevent potential runtime errors.As
listItemColorsis conditionally rendered, this check might already be in place, but double-checking can prevent issues.
206-206: Themed text styling for item descriptionsApplying conditional classes based on
listItemColorsand usingthemeColor("text-neutral-300")ensures consistent theming.src/core/styles/colors/Colors.stories.tsx (3)
4-4: ImportinguseTheminghookThe
useTheminghook is correctly imported from"../../hooks/useTheming".
109-112: Verify thebaseThemeandthemeparameter valuesIn the
useTheminghook,baseThemeis set to"dark"andthemeis set to"light". Please verify if this is intentional. Typically,baseThemerepresents the default or current theme, andthemerepresents the target or new theme.
116-121: Ensure consistent color mapping in themingIn line 116, you use
{colorSet(["orange-300"], "bg-orange-300")}, displayingorange-300with its background color. In line 120, you use{colorSet(["orange-900"], themeColor("bg-orange-300"))}. Please verify whether["orange-900"]should correspond withthemeColor("bg-orange-900")instead ofthemeColor("bg-orange-300"), to maintain consistency between the color name and the themed background class.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
src/core/hooks/useTheming.tsx (1)
10-23: LGTM: Hook implementation is efficient and clear.The
useTheminghook is well-implemented, usinguseCallbackfor performance optimization and providing a clear, focused API. The logic for determining theme color is concise and effective.Consider adding a type annotation for the return value of the hook for improved type safety and documentation:
const useTheming = ({ baseTheme = "dark", theme = "dark", }: UseThemingProps): { themeColor: (color: ColorClass) => ColorClass } => { // ... existing implementation ... };src/core/Icon/EncapsulatedIcon.tsx (2)
21-24: LGTM with a suggestion: Consider making baseTheme configurable.The implementation of
useTheminghook looks good and aligns with the PR objectives. However, consider making thebaseThemeconfigurable instead of hardcoding it to "dark". This would increase the component's flexibility for different use cases.- baseTheme: "dark", + baseTheme: theme === "light" ? "light" : "dark",This change would allow the component to adapt to both light and dark base themes more dynamically.
32-32: LGTM with a readability suggestion.The use of
themeColorfunction and conditional gradient direction look good. These changes align with the PR objectives of reducing generated Tailwind classes and enhancing theming capabilities.To improve readability, consider extracting the className into a separate variable:
const outerClassName = `p-1 rounded-lg ${ theme === "light" ? "bg-gradient-to-t" : "bg-gradient-to-b" } ${themeColor("from-neutral-900")} ${className ?? ""}`; return ( <div className={outerClassName} style={{ width: numericalSize, height: numericalSize }} > {/* ... */} </div> );This change would make the JSX more readable and easier to maintain.
src/core/Tooltip.tsx (2)
169-169: LGTM: Icon color prop updated to use new theming approach.The Icon color prop has been correctly updated to use the themeColor function from the useTheming hook. This change is consistent with the new theming approach introduced in this PR.
Minor suggestion: The template literal might be unnecessary for a single expression. Consider simplifying it to:
color={themeColor("text-neutral-800")}
190-193: LGTM: Tooltip className updated to use new theming approach. Consider improving readability.The tooltip's className has been correctly updated to use the themeColor function for background and text colors, which is consistent with the new theming approach.
However, the current implementation might be hard to read due to the long line and multiple embedded expressions. Consider refactoring for improved readability:
className={` ${themeColor("bg-neutral-1000")} ${themeColor("text-neutral-200")} ui-text-p3 font-medium p-16 ${interactive ? "" : "pointer-events-none"} rounded-lg absolute ${tooltipProps?.className ?? ""} ${fadeOut ? "animate-[tooltipExit_0.25s_ease-in-out]" : "animate-[tooltipEntry_0.25s_ease-in-out]" } `}This approach separates each class or condition onto its own line, making it easier to read and maintain.
scripts/compute-colors.ts (1)
62-69: Include Error Object in Catch Block for Detailed LoggingIn the catch block, the error object is not captured, which limits the usefulness of the error message. By including the error parameter, you provide more context, making it easier to debug issues when they occur.
Apply this diff to include the error object:
try { fs.writeFileSync(outputPath, JSON.stringify(flippedMatches)); console.log( `🎨 Tailwind theme classes have been computed and written to JSON files.`, ); - } catch { - console.error(`Error persisting computed colors.`); + } catch (error) { + console.error(`Error persisting computed colors:`, error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- .gitignore (1 hunks)
- package.json (1 hunks)
- scripts/compute-colors.ts (1 hunks)
- src/core/Icon/EncapsulatedIcon.tsx (2 hunks)
- src/core/Pricing/PricingCards.tsx (11 hunks)
- src/core/ProductTile.tsx (4 hunks)
- src/core/Tooltip.tsx (4 hunks)
- src/core/hooks/useTheming.tsx (1 hunks)
- src/core/styles/colors/Colors.stories.tsx (2 hunks)
- src/core/styles/colors/utils.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- package.json
🧰 Additional context used
🔇 Additional comments (33)
src/core/hooks/useTheming.tsx (4)
1-3: LGTM: Imports are appropriate and well-organized.The imports are relevant to the hook's functionality, utilizing React's
useCallbackfor optimization and local type definitions and utility functions for better code organization.
5-8: LGTM: Type definition is clear and flexible.The
UseThemingPropstype is well-defined, with optional properties allowing for flexible usage of the hook. The use of the customThemetype promotes type safety.
25-25: LGTM: Export statement is appropriate.The default export of the
useTheminghook follows common practices and is suitable for a single hook in the file.
1-25: Great implementation of the useTheming hook!This new file introduces a well-structured and efficient
useTheminghook that aligns perfectly with the PR objectives. The implementation is clean, well-typed, and follows React best practices. By centralizing theme-based color logic, this hook should effectively help reduce generated Tailwind classes across the application.The hook provides a simple and flexible API for theme-based color manipulation, which should make it easy to integrate into existing components. Great job on improving the theming capabilities of the application!
src/core/Icon/EncapsulatedIcon.tsx (3)
3-3: LGTM: Import changes align with PR objectives.The addition of the
useTheminghook import and removal ofdetermineThemeColorare consistent with the PR's goal of enhancing theming capabilities.
36-36: LGTM: Consistent use of themeColor function.The update to use the
themeColorfunction in the inner div's className is consistent with the new theming approach and aligns with the PR objectives of reducing generated Tailwind classes.
Line range hint
1-46: Overall assessment: Changes successfully implement new theming approach.The modifications to
EncapsulatedIcon.tsxeffectively implement the new theming approach using theuseTheminghook. These changes align well with the PR objectives of adding theuseTheminghook and reducing generated Tailwind classes.Key points:
- Successful integration of the
useTheminghook.- Consistent use of the
themeColorfunction throughout the component.- Reduction in hardcoded color classes, potentially optimizing Tailwind class generation.
The suggestions provided for making the
baseThemeconfigurable and improving readability are minor and optional improvements. Overall, the implementation achieves its goals and enhances the component's theming capabilities.src/core/Tooltip.tsx (3)
14-14: LGTM: Importing useTheming hook aligns with PR objectives.The addition of the useTheming hook and removal of the determineThemeColor utility function aligns well with the PR objective of adding a new theming hook and reducing generated Tailwind classes. This change centralizes the theming logic, potentially making it easier to manage and update theme-related functionality across the application.
Line range hint
1-203: Overall, great job on implementing the useTheming hook!The changes in this file effectively implement the new useTheming hook, replacing the previous determineThemeColor function. This aligns well with the PR objectives of adding a new theming hook and potentially reducing generated Tailwind classes.
The theming logic is now more centralized and consistently applied throughout the component, which should improve maintainability and flexibility in theme management. The minor suggestions provided in the review comments can further enhance code readability and optimization.
Great work on improving the theming system in the Tooltip component!
41-44: LGTM: useTheming hook implementation looks good.The implementation of the useTheming hook is correct and aligns with the PR objectives. It replaces the previous determineThemeColor function, potentially providing more flexibility in theme management.
However, please verify if setting the baseTheme to "light" is intentional, as it might affect the default appearance of the tooltip.
To ensure consistency across the codebase, let's check if "light" is used as the baseTheme in other components:
✅ Verification successful
LGTM: useTheming hook implementation looks good.
The implementation of the
useTheminghook is correct and aligns with the PR objectives. It replaces the previousdetermineThemeColorfunction, potentially providing more flexibility in theme management.No other components are using
baseThemeset to"light", ensuring that this configuration is specific to the tooltip component and maintains the intended default appearance.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for useTheming hook usage with baseTheme set to "light" rg --type typescript --type tsx 'useTheming.*baseTheme:\s*"light"'Length of output: 103
Script:
#!/bin/bash # Search for useTheming hook usage with baseTheme set to "light" rg --type ts --type tsx 'useTheming.*baseTheme:\s*"light"'Length of output: 88
Script:
#!/bin/bash # Search for useTheming hook usage with baseTheme set to "light" using glob patterns rg 'useTheming.*baseTheme:\s*"light"' --glob '*.ts' --glob '*.tsx'Length of output: 68
src/core/styles/colors/utils.ts (2)
1-10: Imports are correct and necessaryAll imported color arrays and the
ColorClasstype are appropriately used in the code.
15-23: Extents mapping is accurately definedThe
extentsconstant correctly maps each color to the length of its corresponding color array.src/core/ProductTile.tsx (2)
5-5: Import ofuseThemingis appropriateThe
useTheminghook is correctly imported from./hooks/useTheming.
22-25: Proper implementation ofuseTheminghookThe usage of
useThemingto obtainthemeColorwith the specifiedbaseThemeand dynamicthemebased on theselectedprop is appropriate and enhances theming capabilities.src/core/styles/colors/Colors.stories.tsx (1)
116-121: Verify color consistency in theming examples.In the theming examples, you are using `"orange-300"` in the first `colorSet` and `"orange-900"` in the second. Given that you're applying `themeColor("bg-orange-300")` to the second one, please verify if both should be using the same color to demonstrate the theming effect on the same base color.
src/core/Pricing/PricingCards.tsx (18)
9-9: ImportuseThemingHookThe
useTheminghook is correctly imported from"../hooks/useTheming".
48-51: InitializethemeColorwithuseThemingThe
useTheminghook is properly used to initializethemeColor, withbaseThemeset to"dark"andthemepassed as a prop.
62-62: UpdateIconComponent withthemeColorThe
themeColorfunction is correctly used to apply thetext-neutral-500color to theadditionalCSSprop of theIconcomponent.
95-95: ApplythemeColorto Card ContainerThe
themeColorfunction is appropriately used to determine the border color class based on theborderprop in the card container'sclassName.
105-105: Set Background Color withthemeColorThe
themeColorfunction is correctly applied to set background color classes in theclassName, enhancing theming consistency.
113-113: ApplythemeColorto Title TextThe
themeColorfunction is properly used to set the text color of the title, ensuring it adapts to the current theme.
127-127: ApplythemeColorto Description TextThe
themeColorfunction is correctly applied to set the text color of the description, maintaining theming consistency.
139-139: Set Price Amount Text Color withthemeColorThe
themeColorfunction is appropriately used to apply thetext-neutral-000color to the price amount text.
143-145: ApplythemeColorto Price ContentThe
themeColorfunction is correctly used to set the text color of the price content within the<div>element.
152-152: UpdateFeaturedLinkStyling withthemeColorThe
themeColorfunction is properly applied to set the background and text colors in theadditionalCSSprop of theFeaturedLinkcomponent.
162-164: ApplythemeColorto Horizontal RuleThe
themeColorfunction is correctly used to set the border color of the<hr>element, ensuring theme consistency.
172-172: Set Section Title Color withthemeColorThe
themeColorfunction is appropriately applied to set the text color of section titles, enhancing theming.
181-181: ApplythemeColorto Item Container BackgroundThe
themeColorfunction is correctly used to set the background color for alternating items in the item container.
186-186: Set Item Text Color withthemeColorThe
themeColorfunction is properly applied to the item text, ensuring consistent theming across items.
206-206: ApplythemeColorto Item Description TextThe
themeColorfunction is correctly used to set the text color of the item descriptions, maintaining consistency with the theme.
218-218: UpdateFeaturedLinkStyling withthemeColorThe
themeColorfunction is appropriately applied in theadditionalCSSprop to set text colors for theFeaturedLinkcomponent.
220-220: Safely AccesslistItemColors.foregroundwith Optional ChainingThe use of optional chaining
listItemColors?.foregroundensures that iflistItemColorsis undefined, it defaults to"text-white", preventing potential errors.
227-227: Set Placeholder Text Color withthemeColorThe
themeColorfunction is correctly applied to set the text color of the placeholder text, ensuring it aligns with the current theme.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- scripts/compute-colors.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/compute-colors.ts (2)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:40-42 Timestamp: 2024-10-14T14:51:08.699Z Learning: In `scripts/compute-colors.ts`, when constructing the regex pattern to match `themeColor` usages, the missing separators between `joinedVariants` and `joinedPrefixes` are intentional.Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:37-37 Timestamp: 2024-10-14T14:50:06.863Z Learning: In `scripts/compute-colors.ts`, when searching for `themeColor` usages, only `.tsx` files need to be processed; there is no need to include other file types such as `.ts`, `.jsx`, or `.js`.
🔇 Additional comments (5)
scripts/compute-colors.ts (5)
1-14: LGTM: Import statements and constant declarationsThe import statements and constant declarations look good. The introduction of
invertTailwindClassVariantand the definition of directory and output paths are appropriate for the script's purpose.
56-60: LGTM: Main script executionThe main script execution looks good. It correctly calls
findStringInFileswith the directory path and appliesinvertTailwindClassVariantto each match, which aligns with the script's purpose of computing and inverting theme color classes.
62-72: LGTM: File writing logic with proper error handlingThe file writing logic looks good. It includes appropriate error handling for the file writing operation and creates the output directory if it doesn't exist. The inclusion of the error object in the catch block's console.error message is a good practice for debugging.
1-72: Overall assessment: Significant refactor with good improvementsThe script has undergone a substantial refactor, shifting its focus to finding and processing theme color strings in
.tsxfiles. The new implementation introduces useful utility functions and leverages existing color utilities effectively. Key improvements include:
- Introduction of the
findStringInFilesfunction for recursive file searching.- Use of
invertTailwindClassVariantfor color processing.- Improved error handling in the file writing logic.
While the overall structure and purpose of the script have changed, the implementation is logical and aligned with the new requirements. To further enhance the script, consider:
- Centralizing color definitions for better maintainability.
- Adding more comprehensive error handling, especially in file system operations.
These suggestions aside, the refactored script appears to be a solid improvement that should effectively serve its new purpose.
15-23: 🛠️ Refactor suggestionConsider importing colors from a central configuration
The
colorsarray is currently hardcoded within the script. To enhance maintainability and ensure consistency across the codebase, consider importing color definitions from a central configuration or constants file.
jamiehenson
force-pushed
the
reduce-tw-class-generation
branch
from
87df035 to
a105c03
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
src/core/styles/colors/utils.ts (1)
8-10: Enhance input validation for robustness.While the current check ensures the class name has at least 3 parts, consider adding more comprehensive validation:
- Check if
colorexists inColorClassColorGroups.- Validate that
variantis a valid number followed by "00".Example:
if (splitMatch.length < 3 || !(color in colorGroupLengths) || !/^\d+00$/.test(variant)) { throw new Error(`Invalid className format: ${className}`); }This will catch more potential errors early and provide more informative error messages.
src/core/Icon/EncapsulatedIcon.tsx (1)
32-32: LGTM: Updated class names with themeColor function.The class names have been successfully updated to use the
themeColorfunction, which aligns with the PR objective of reducing generated Tailwind classes and implementing the new theming approach.Suggestion for improvement:
Consider extracting the complex class name logic into a separate function to enhance readability. For example:const getOuterDivClassName = (theme: Theme, themeColor: Function, className?: string) => { return `p-1 rounded-lg ${theme === "light" ? "bg-gradient-to-t" : "bg-gradient-to-b"} ${themeColor("from-neutral-900")} ${className ?? ""}`; }; const getInnerDivClassName = (themeColor: Function, innerClassName?: string) => { return `flex items-center justify-center rounded-lg ${themeColor("bg-neutral-1100")} ${innerClassName ?? ""}`; };Then use these functions in your JSX:
<div className={getOuterDivClassName(theme, themeColor, className)} style={{ width: numericalSize, height: numericalSize }} > <div className={getInnerDivClassName(themeColor, innerClassName)} style={{ height: numericalSize - 2 }} > {/* ... */} </div> </div>This approach would make the component more readable and easier to maintain.
Also applies to: 36-36
src/core/ProductTile.tsx (5)
22-25: LGTM: Proper usage of useTheming hook.The implementation of the
useTheminghook is correct and aligns well with the component's theming needs. The dynamic theme selection based on theselectedprop is a good approach for conditional theming.Consider destructuring the
selectedprop in the component parameters for improved readability:const ProductTile = ({ name, selected, currentPage, className, onClick, }: ProductTileProps) => { const { themeColor } = useTheming({ baseTheme: "dark", theme: selected ? "light" : "dark", }); // ... };This minor change would make the code slightly more concise and easier to follow.
30-30: Consider refactoring the className for improved readability.The update to use
themeColoris correct and aligns with the new theming approach. However, the className string is becoming complex. Consider refactoring it for improved readability:const containerClasses = [ 'rounded-lg p-12 flex flex-col gap-8 transition-colors', themeColor("bg-neutral-1200"), selected ? '' : 'hover:bg-neutral-1100', className ].filter(Boolean).join(' '); return ( <div className={containerClasses} onClick={onClick} > {/* ... */} </div> );This approach separates the class logic from the JSX, making it easier to read and maintain.
41-41: Consider refactoring the className for improved readability.The update to use
themeColoris correct and aligns with the new theming approach. However, the className string is becoming complex. Consider refactoring it for improved readability:const paragraphClasses = [ unavailable ? 'ui-text-p2' : 'ui-text-p3', themeColor("text-neutral-300"), 'font-medium' ].join(' '); return ( <p className={paragraphClasses}> Ably{" "} </p> );This approach separates the class logic from the JSX, making it easier to read and maintain.
46-46: Consider refactoring the className for improved readability.The update to use
themeColoris correct and aligns with the new theming approach. However, the className string is becoming complex. Consider refactoring it for improved readability:const labelClasses = [ 'ui-text-p2', themeColor("text-neutral-000"), 'font-bold', unavailable ? '' : 'mt-[-3px]' ].filter(Boolean).join(' '); return ( <p className={labelClasses}> {label} </p> );This approach separates the class logic from the JSX, making it easier to read and maintain.
66-66: Consider refactoring the additionalCSS prop for improved readability.The update to use
themeColoris correct and aligns with the new theming approach. However, the additionalCSS string is quite long. Consider refactoring it for improved readability:const featuredLinkClasses = [ 'ui-btn-secondary bg-transparent hover:bg-transparent', 'w-full hover:text-neutral-1300 mt-8 text-center inline-block', themeColor("text-neutral-000") ].join(' '); return ( <FeaturedLink additionalCSS={featuredLinkClasses} iconColor="text-orange-600" url={link} > {currentPage ? "View docs" : "Explore"} </FeaturedLink> );This approach separates the class logic from the JSX, making it easier to read and maintain.
src/core/styles/colors/types.ts (1)
Line range hint
1-194: Overall feedback: Improved color management with minor adjustments needed.The changes in this file generally improve the color management system by introducing more structured and type-safe approaches. The new
colorsconstant,ColorClassColorGroupstype, andcolorGroupLengthsobject enhance the organization and usability of color-related code.However, there are a few points that need attention:
- Clarify the rationale behind removing
secondaryandaliasedcolors fromcolorRoles.- Address the potential inconsistency in how color group lengths are derived for different color groups.
Once these points are addressed, the changes will significantly enhance the color management system in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- .gitignore (1 hunks)
- package.json (1 hunks)
- scripts/compute-colors.ts (1 hunks)
- src/core/Icon/EncapsulatedIcon.tsx (2 hunks)
- src/core/Pricing/PricingCards.tsx (11 hunks)
- src/core/ProductTile.tsx (4 hunks)
- src/core/Tooltip.tsx (4 hunks)
- src/core/hooks/useTheming.tsx (1 hunks)
- src/core/styles/colors/Colors.stories.tsx (6 hunks)
- src/core/styles/colors/types.ts (2 hunks)
- src/core/styles/colors/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .gitignore
- package.json
- src/core/Pricing/PricingCards.tsx
- src/core/Tooltip.tsx
- src/core/hooks/useTheming.tsx
🧰 Additional context used
📓 Learnings (3)
scripts/compute-colors.ts (2)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:37-37 Timestamp: 2024-10-14T14:50:06.863Z Learning: In `scripts/compute-colors.ts`, when searching for `themeColor` usages, only `.tsx` files need to be processed; there is no need to include other file types such as `.ts`, `.jsx`, or `.js`.Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:40-42 Timestamp: 2024-10-14T14:51:08.699Z Learning: In `scripts/compute-colors.ts`, when constructing the regex pattern to match `themeColor` usages, the missing separators between `joinedVariants` and `joinedPrefixes` are intentional.src/core/ProductTile.tsx (1)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: src/core/ProductTile.tsx:30-30 Timestamp: 2024-10-14T15:13:43.163Z Learning: In `src/core/ProductTile.tsx`, prefer not to use utility libraries like `clsx` for conditional class names.src/core/styles/colors/types.ts (2)
Learnt from: jamiehenson PR: ably/ably-ui#486 File: src/core/styles/colors/types.ts:8-15 Timestamp: 2024-09-18T14:08:15.229Z Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.Learnt from: jamiehenson PR: ably/ably-ui#486 File: src/core/styles/colors/types.ts:8-15 Timestamp: 2024-10-08T15:37:26.766Z Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.
🔇 Additional comments (23)
src/core/styles/colors/utils.ts (3)
1-1: LGTM: Import changes align with function modifications.The updated imports correctly reflect the changes in the file's functionality, removing unused imports and adding necessary types and constants for the new
invertTailwindClassVariantfunction.
16-19: Please clarify the neutral color adjustment.The calculation of
flippedVariantincludes a special case for the "neutral" color:(color === "neutral" ? 1 : -1)Could you provide a comment explaining why this adjustment is necessary for neutral colors? This would improve the code's maintainability and help future developers understand the logic behind this special case.
6-22: LGTM: Well-implemented function with minor suggestions.The
invertTailwindClassVariantfunction is well-structured and correctly implements the inversion of Tailwind CSS color variants. It handles the parsing of class names, calculates the flipped variant, and constructs the new class name effectively.Consider the following minor improvements:
- Add more comprehensive input validation as suggested earlier.
- Use a more robust approach for parsing the variant number.
- Add a comment explaining the neutral color adjustment.
- Consider adding unit tests to verify the function's behavior with various inputs.
Overall, good job on implementing this new functionality!
src/core/Icon/EncapsulatedIcon.tsx (3)
3-3: LGTM: Import statement for useTheming hook.The import statement for the
useTheminghook is correctly implemented. This change aligns with the PR objective of adding theuseTheminghook and reducing generated Tailwind classes.
Line range hint
1-46: Summary: Successful implementation of useTheming hook with minor suggestions for improvementThe changes in this file successfully implement the
useTheminghook and update the class names to use the new theming approach. This aligns well with the PR objectives of adding theuseTheminghook and reducing generated Tailwind classes.Key points:
- The
useTheminghook is correctly imported and implemented.- Class names are updated to use the
themeColorfunction from the hook.- The changes contribute to more dynamic and efficient theming.
Suggestions for further improvement:
- Consider the implications of setting
baseTheme: "dark"in theuseTheminghook call.- Extract complex class name logic into separate functions for better readability and maintainability.
Overall, the changes are approved with minor suggestions for refinement.
21-24: LGTM: useTheming hook implementation with a minor consideration.The
useTheminghook is correctly implemented and used. It aligns with the PR objective of adding the hook for theming purposes.However, consider the implications of setting
baseTheme: "dark":
- This might affect the default behavior of the component.
- Ensure this aligns with the intended design system and doesn't introduce unexpected behavior for light themes.
To verify the impact of this change, let's check for other usages of
useThemingin the codebase:✅ Verification successful
Action Required: Corrected command to locate other
useThemingusages.To accurately identify all instances of the
useTheminghook within TypeScript files, please run the following script:
LGTM: useTheming hook implementation confirmed as consistent.
The
useTheminghook is correctly implemented and consistently used across the codebase. SettingbaseTheme: "dark"aligns with other components, ensuring uniform theming behavior.
- Verified that
useThemingis utilized appropriately in multiple files with bothdarkandlightbase themes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of useTheming hook rg --type typescript 'useTheming\(' -C 3Length of output: 77
Script:
#!/bin/bash # Description: Check for other usages of useTheming hook in TypeScript files rg 'useTheming\(' --glob '*.ts' --glob '*.tsx' -C 3Length of output: 1951
src/core/ProductTile.tsx (2)
5-5: LGTM: Import changes align with the new theming approach.The addition of the
useTheminghook import and removal of theColorClassanddetermineThemeColorimports reflect the shift towards using a custom hook for theming. This change aligns well with the PR objective of adding a useTheming hook and reducing generated TW classes.
Line range hint
1-79: Overall assessment: Good implementation of the new theming approach.The changes in this file successfully implement the new
useTheminghook and consistently apply the new theming approach throughout theProductTilecomponent. This aligns well with the PR objectives of adding a useTheming hook and reducing generated TW classes.Key points:
- The import statements have been updated correctly.
- The
useTheminghook is properly implemented and used.- All instances of theme-related class names now use the
themeColorfunction.While the changes are functionally correct, there are opportunities to improve code readability, particularly in the construction of class name strings. Consider implementing the suggested refactors to enhance maintainability.
Great job on implementing the new theming system!
scripts/compute-colors.ts (6)
3-4: LGTM: Import statements are correctThe new import for
invertTailwindClassVariantis appropriate for the changes in the file's functionality. The existing import forprefixesandvariantsis maintained.
6-11: LGTM: Path definitions are well-structuredThe
directoryPathandoutputPathare correctly defined usingpath.join, ensuring cross-platform compatibility. The output path is appropriately set within the project structure.
25-71: LGTM: Well-structured function with proper error handlingThe
findStringInFilesfunction is well-implemented with:
- Recursive directory traversal
- Proper error handling for file system operations
- Correct file type filtering (.tsx files only)
The overall structure and error handling approach are commendable.
74-77: LGTM: Efficient processing of matchesThe code efficiently processes the matches found by
findStringInFiles:
- It calls the function with the correct
directoryPath.- It then maps the results using
invertTailwindClassVariant, which aligns with the purpose of this script.This approach is concise and effective.
80-88: LGTM: Robust file writing with proper checksThis section demonstrates good practices:
- It checks and creates the output directory if it doesn't exist.
- It writes the processed matches to the JSON file.
- It provides an informative success message.
The implementation ensures that the file writing process is robust and user-friendly.
89-91: LGTM: Appropriate error handlingThe error handling for the file writing process is well-implemented:
- It catches any potential errors during the operation.
- It logs the error with details, which is crucial for debugging.
This approach is consistent with the error handling used throughout the file and provides valuable information in case of failures.
src/core/styles/colors/types.ts (2)
22-32: LGTM! Improved color group management.The introduction of the
colorsconstant and theColorClassColorGroupstype is a positive change. It centralizes the definition of color group names and provides type safety for their usage throughout the codebase. This addition aligns well with the existing color definitions and enhances the overall type system for colors.
179-185: Approve renaming, but clarify color removals.The renaming of
colorNamestocolorRolesis a good change that better reflects the purpose of these color groups in the UI. However, the removal ofsecondaryandaliasedcolors fromcolorRolesrequires clarification:
- What is the rationale behind removing these color groups?
- Are there any potential impacts on existing code that might rely on these color groups?
- If these colors are still needed, where are they being managed now?
Please provide some context on these changes to ensure they align with the overall color management strategy.
src/core/styles/colors/Colors.stories.tsx (7)
2-4: Updated imports to reflect new theming systemThe import statements correctly include
colorRolesanduseTheming, aligning with the updated theming approach.
50-50: Updated 'NeutralColors' to use 'colorRoles.neutral'The
NeutralColorscomponent now utilizescolorRoles.neutralto render the color set, ensuring consistency with the new color roles structure.
65-65: Updated 'OrangeColors' to use 'colorRoles.orange'The
OrangeColorscomponent now utilizescolorRoles.orangefor rendering, reflecting the updated color role definitions.
80-80: Updated 'SecondaryColors' to use 'colorRoles.secondary'The
SecondaryColorscomponent correctly usescolorRoles.secondary, maintaining consistency with the updated theming system.
94-94: Updated 'GUIColors' to use 'colorRoles.gui'The
GUIColorscomponent now referencescolorRoles.gui, aligning with the new color roles configuration.
107-124: Implemented theming in 'DynamicTheming' component usinguseThemingThe
DynamicThemingcomponent correctly implements theuseTheminghook and utilizes thethemeColorfunction to dynamically adjust color classes based on the theme context.
129-129: Updated documentation to reflect new theming approachThe story description accurately explains the usage of the
useTheminghook andthemeColorfunction for generating alternative colors based on theme differences.
jamiehenson
force-pushed
the
reduce-tw-class-generation
branch
from
a105c03 to
1133a22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
src/core/styles/colors/utils.ts (2)
6-11: Enhance error message for invalid class format.The error handling for invalid class format is a good practice. However, consider making the error message more descriptive to aid in debugging.
Suggest updating the error message as follows:
- throw new Error("Invalid TW class format"); + throw new Error(`Invalid Tailwind class format: ${className}`);This change will provide more context about which class caused the error.
12-27: Approve implementation with suggestions for improvement.The overall implementation of
invertTailwindClassVariantis well-structured and includes good error handling. Here are some suggestions for further improvement:
- The function assumes all variants end with "00", which might not always be true in Tailwind CSS (e.g., "50" variants). Consider making this more robust:
const numericalVariant = parseInt(variant.replace(/00$/, ''), 10);
- Enhance the error message for invalid variant values:
- throw new Error(`Invalid variant value in TW class: ${className}`); + throw new Error(`Invalid variant value '${variant}' in Tailwind class: ${className}`);
- Consider adding a check to ensure the color exists in
colorGroupLengths:if (!(color in colorGroupLengths)) { throw new Error(`Unknown color '${color}' in Tailwind class: ${className}`); }These changes will make the function more robust and provide more helpful error messages.
🧰 Tools
🪛 Biome
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
src/core/ProductTile.tsx (4)
22-25: Approved: Dynamic theming with useTheming hookThe implementation of the
useTheminghook provides a more flexible and centralized approach to theme management. It correctly uses theselectedprop to determine the appropriate theme.Consider destructuring the
selectedprop at the top of the component for improved readability:const ProductTile = ({ name, selected, currentPage, className, onClick, }: ProductTileProps) => { + const { selected } = props; const { themeColor } = useTheming({ baseTheme: "dark", theme: selected ? "light" : "dark", }); // ...
30-30: Improve className structure while maintaining new theming approachThe use of
themeColorfunction for applying the background color is correct and aligns with the new theming approach. However, the string concatenation for className could be improved for better readability and maintainability.Consider using template literals for better readability:
- className={`rounded-lg p-12 flex flex-col gap-8 transition-colors ${themeColor("bg-neutral-1200")} ${selected ? "" : "hover:bg-neutral-1100"} ${className ?? ""}`} + className={` + rounded-lg p-12 flex flex-col gap-8 transition-colors + ${themeColor("bg-neutral-1200")} + ${selected ? "" : "hover:bg-neutral-1100"} + ${className ?? ""} + `}This change improves readability while maintaining the functionality.
41-41: Improve className structure in paragraph elementsThe use of
themeColorfunction in both paragraph elements is correct and consistent with the new theming approach. However, the string concatenation for classNames could be improved for better readability and maintainability.Consider using template literals for better readability in both paragraph elements:
For the first paragraph (line 41):
- className={`${unavailable ? "ui-text-p2" : "ui-text-p3"} ${themeColor("text-neutral-300")} font-medium`} + className={` + ${unavailable ? "ui-text-p2" : "ui-text-p3"} + ${themeColor("text-neutral-300")} + font-medium + `}For the second paragraph (line 46):
- className={`ui-text-p2 ${themeColor("text-neutral-000")} font-bold ${unavailable ? "" : "mt-[-3px]"}`} + className={` + ui-text-p2 + ${themeColor("text-neutral-000")} + font-bold + ${unavailable ? "" : "mt-[-3px]"} + `}These changes improve readability while maintaining the functionality.
Also applies to: 46-46
66-66: Improve additionalCSS structure in FeaturedLink componentThe use of
themeColorfunction in theadditionalCSSprop is correct and consistent with the new theming approach. However, the string concatenation could be improved for better readability and maintainability.Consider using template literals for better readability:
- additionalCSS={`ui-btn-secondary bg-transparent hover:bg-transparent w-full hover:text-neutral-1300 mt-8 text-center inline-block ${themeColor("text-neutral-000")}`} + additionalCSS={` + ui-btn-secondary bg-transparent hover:bg-transparent + w-full hover:text-neutral-1300 mt-8 text-center inline-block + ${themeColor("text-neutral-000")} + `}This change improves readability while maintaining the functionality.
scripts/compute-colors.ts (1)
66-70: LGTM: Clear execution logic with a minor suggestionThe main execution logic is clear and concise. It correctly uses the
findStringInFilesfunction and processes the results withinvertTailwindClassVariant.Consider adding a brief comment explaining the purpose of
invertTailwindClassVariantfor better code documentation. For example:// Invert theme colors for dark/light mode compatibility const flippedMatches = matches.map((match) => invertTailwindClassVariant(match) );src/core/styles/colors/types.ts (1)
Line range hint
1-194: Reminder: Maintain color-related consistencyWhile the current changes don't directly modify
ColorClassVariantsorColorClassPrefixes, it's important to keep in mind the previous learning:When updating
ColorClassVariantsandColorClassPrefixes, only include variants and prefixes that relate to colour classes.This principle should be applied in future updates to this file to maintain consistency in color-related type definitions.
src/core/Pricing/PricingCards.tsx (3)
95-95: LGTM with a minor suggestion: Theming applied to borderThe
themeColorfunction is correctly used to apply the theme-based color to the border, and the nullish coalescing operator provides a good fallback. However, this line is quite long and might benefit from being split for better readability.Consider refactoring like this for improved readability:
className={` relative border ${themeColor(borderClasses(border?.color)?.border ?? "border-neutral-1100")} ${border?.style ?? ""} flex-1 px-24 py-32 flex flex-col gap-24 rounded-2xl group ${delimiter ? "@[520px]:flex-row @[920px]:flex-col" : ""} min-w-[272px] backdrop-blur `}This change would make the code easier to read and maintain without changing its functionality.
105-105: LGTM with a minor suggestion: Theming applied to backgroundThe
themeColorfunction is correctly used to apply the theme-based color to the background, and the conditional hover effects are maintained. However, this line is quite long and might benefit from being split for better readability.Consider refactoring like this:
className={` absolute z-0 top-0 left-0 w-full h-full rounded-2xl ${themeColor("bg-neutral-1300")} ${!delimiter ? `${themeColor("group-hover:bg-neutral-1200")} group-hover:opacity-100` : "" } transition-[colors,opacity] opacity-25 `}This change would make the code easier to read and maintain without changing its functionality.
152-152: Theming applied to FeaturedLink, but improvements neededThe
themeColorfunction is correctly used to apply theme-based colors to the background and text of the FeaturedLink. However, there are a few points to consider:
- The line is very long, making it hard to read and maintain.
- The hover state text color is hardcoded and not using
themeColor.Consider refactoring like this for improved readability and consistency:
additionalCSS={` text-center ui-btn ${themeColor("bg-neutral-000")} ${themeColor("text-neutral-1300")} ${themeColor("hover:text-neutral-000")} px-24 !py-12 ${cta.className ?? ""} cursor-pointer `}This change would make the code easier to read and maintain, and ensure consistent use of theming throughout, including in the hover state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- .gitignore (1 hunks)
- package.json (1 hunks)
- scripts/compute-colors.ts (1 hunks)
- src/core/Icon/EncapsulatedIcon.tsx (2 hunks)
- src/core/Pricing/PricingCards.tsx (11 hunks)
- src/core/ProductTile.tsx (4 hunks)
- src/core/Tooltip.tsx (4 hunks)
- src/core/hooks/useTheming.tsx (1 hunks)
- src/core/styles/colors/Colors.stories.tsx (6 hunks)
- src/core/styles/colors/types.ts (2 hunks)
- src/core/styles/colors/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .gitignore
- package.json
- src/core/Icon/EncapsulatedIcon.tsx
- src/core/Tooltip.tsx
- src/core/hooks/useTheming.tsx
🧰 Additional context used
📓 Learnings (3)
scripts/compute-colors.ts (2)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:37-37 Timestamp: 2024-10-14T14:50:06.863Z Learning: In `scripts/compute-colors.ts`, when searching for `themeColor` usages, only `.tsx` files need to be processed; there is no need to include other file types such as `.ts`, `.jsx`, or `.js`.Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:40-42 Timestamp: 2024-10-14T14:51:08.699Z Learning: In `scripts/compute-colors.ts`, when constructing the regex pattern to match `themeColor` usages, the missing separators between `joinedVariants` and `joinedPrefixes` are intentional.src/core/ProductTile.tsx (1)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: src/core/ProductTile.tsx:30-30 Timestamp: 2024-10-14T15:13:43.163Z Learning: In `src/core/ProductTile.tsx`, prefer not to use utility libraries like `clsx` for conditional class names.src/core/styles/colors/types.ts (2)
Learnt from: jamiehenson PR: ably/ably-ui#486 File: src/core/styles/colors/types.ts:8-15 Timestamp: 2024-09-18T14:08:15.229Z Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.Learnt from: jamiehenson PR: ably/ably-ui#486 File: src/core/styles/colors/types.ts:8-15 Timestamp: 2024-10-08T15:37:26.766Z Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.
🪛 Biome
src/core/styles/colors/utils.ts
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (24)
src/core/styles/colors/utils.ts (2)
1-5: LGTM: Import changes and existing function.The updated imports reflect the changes in how color-related data is managed. The
convertTailwindClassToVarfunction remains unchanged and continues to serve its purpose.
1-27: Summary: Alignment with PR objectives and overall improvements.The changes in this file align well with the PR objectives of "[WEB-4025] Add useTheming hook, reduce generated TW classes". The removal of
determineThemeColorand the addition ofinvertTailwindClassVariantcontribute to the goal of dynamic theming and potentially reducing generated Tailwind classes.The new
invertTailwindClassVariantfunction provides a mechanism for inverting Tailwind color variants, which can be useful in implementing dynamic theming. The function includes error handling and complex logic to handle various Tailwind class formats.While the implementation is generally solid, consider the suggested improvements to make the function more robust and error messages more informative. These enhancements will contribute to better maintainability and easier debugging in the future.
🧰 Tools
🪛 Biome
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
src/core/ProductTile.tsx (2)
5-5: Approved: Shift to useTheming hookThe import of
useTheminghook replaces the previous imports ofColorClassanddetermineThemeColor. This change aligns with the PR objective of adding auseTheminghook and potentially optimizing the theming approach.
Line range hint
1-78: Summary: Successful implementation of new theming approachThe changes in this file successfully implement the new theming approach using the
useTheminghook. This aligns well with the PR objectives and provides more flexibility in theme management. ThethemeColorfunction is consistently used throughout the component, replacing the previousdetermineThemeColorutility.Key improvements:
- Centralized theme management with
useTheminghook- Consistent application of
themeColorfunction across the component- Potential for reduced generated Tailwind classes (though this would need to be verified)
While the functionality is correct, there's room for improving the readability of className and additionalCSS structures, as suggested in the previous comments.
Overall, these changes represent a positive step towards more efficient and flexible theming in the component.
scripts/compute-colors.ts (4)
3-14: LGTM: Imports and constants are well-definedThe imports and constants are appropriately set up for the task. The use of
path.joinfor directory paths ensures cross-platform compatibility, and joining arrays to create regex patterns is an efficient approach.
17-63: LGTM: Well-structured function with proper error handlingThe
findStringInFilesfunction is well-implemented with appropriate error handling for file system operations. It correctly processes only.tsxfiles as per the project requirements. The regex pattern construction aligns with the intentional design mentioned in the previous feedback.I've taken into account the following learnings:
- Only
.tsxfiles need to be processed when searching forthemeColorusages.- The missing separators in the regex pattern between
joinedVariantsandjoinedPrefixesare intentional.
72-82: LGTM: Robust file writing logic with proper error handlingThe file writing logic is well-implemented:
- It ensures the output directory exists before writing the file, preventing potential errors.
- Proper error handling is in place for the file writing operation.
- A clear success message is logged upon completion, providing good feedback.
These improvements make the script more robust and user-friendly.
1-82: Overall: Significant improvement in robustness and maintainabilityThis rewrite of
compute-colors.tsbrings several notable improvements:
- Clear focus on finding and processing theme color strings in
.tsxfiles.- Robust error handling throughout the script, especially for file system operations.
- Improved maintainability with well-structured functions and constants.
- Better cross-platform compatibility using
path.joinfor file paths.- Efficient regex pattern construction for finding theme colors.
These changes result in a more reliable and maintainable script for computing theme colors. Great job on the refactoring!
src/core/styles/colors/types.ts (2)
22-32: LGTM: Improved color group managementThe addition of the
colorsconstant andColorClassColorGroupstype enhances the organization of color-related code. This change provides a centralized list of color group names and ensures type safety when referring to these groups throughout the codebase.
186-194: LGTM: Useful addition ofcolorGroupLengthsThe
colorGroupLengthsconstant is a valuable addition, providing easy access to the number of colors in each group. This can be beneficial for various color-related operations in the codebase.The implementation is now consistent with the newly added
colorsconstant, addressing the concern raised in the previous review comment. Each color group's length is derived from its respective color array, ensuring consistency across the color system.src/core/styles/colors/Colors.stories.tsx (6)
2-2: LGTM: Import changes align with PR objectives.The replacement of
colorNameswithcolorRolesand the addition of theuseTheminghook are consistent with the PR's goal of enhancing theming capabilities.Also applies to: 4-4
50-50: LGTM: Consistent use ofcolorRoles.The update to use
colorRoles.neutralis consistent with the import changes and likely reflects a broader restructuring of color definitions.
65-65: LGTM: Consistent updates to color role usage.The changes in OrangeColors and SecondaryColors stories to use
colorRolesinstead ofcolorNamesare consistent with the previous modifications and import changes.Also applies to: 80-80
94-94: LGTM: Consistent update to GUI colors.The change in the GUIColors story to use
colorRoles.guiis consistent with the previous modifications and maintains the overall pattern of updates in this file.
107-124: LGTM: Successful implementation of dynamic theming.The changes in the DynamicTheming story effectively demonstrate the new theming approach:
- The
useTheminghook is correctly implemented withbaseThemeandthemeparameters.- The render method now uses the
themeColorfunction to showcase dynamic color generation.- The JSX structure has been updated to reflect these changes.
The documentation string has been updated to accurately describe the new approach, and the previously reported issue with the missing backtick has been resolved.
Also applies to: 129-129
Line range hint
1-129: Overall: Excellent implementation of new theming approach.The changes in this file successfully achieve the PR objectives:
- Consistent replacement of
colorNameswithcolorRolesacross all color stories.- Introduction and demonstration of the
useTheminghook in the DynamicTheming story.- Updated documentation to reflect the new theming approach.
These changes enhance the theming capabilities of the components while maintaining consistency throughout the file.
src/core/Pricing/PricingCards.tsx (8)
9-9: LGTM: New useTheming hook importedThe import of the
useTheminghook is correctly placed and follows good practices for code organization.
Line range hint
19-19: LGTM: Default theme set to "dark"Setting a default theme to "dark" is a good practice. It ensures that the component always has a valid theme, even if one is not explicitly provided.
48-51: LGTM: useTheming hook implemented correctlyThe
useTheminghook is well-implemented:
- It's called with the correct parameters (
baseThemeandtheme).- The destructuring of
themeColorfrom the hook's return value is clean and follows React best practices.- The "dark"
baseThemealigns with the default theme prop value.This implementation provides a consistent way to manage theming throughout the component.
62-62: LGTM: Theming applied to delimiter iconThe
themeColorfunction is correctly used to apply the theme-based color to the delimiter icon. This change ensures consistency with the new theming approach.
113-113: LGTM: Theming applied to title textThe
themeColorfunction is correctly used to apply the theme-based color to the title text. The use of the nullish coalescing operator provides a good fallback, ensuring that a color is always applied. This change maintains consistency with the new theming approach.
127-127: LGTM: Theming applied to description textThe
themeColorfunction is correctly used to apply the theme-based color to the description text. The use of the nullish coalescing operator provides a good fallback, ensuring that a color is always applied. This change maintains consistency with the new theming approach and preserves the existing styling classes.
139-139: LGTM: Theming applied to price amountThe
themeColorfunction is correctly used to apply the theme-based color to the price amount text. This change ensures consistency with the new theming approach while maintaining the existing typography classes.
143-145: LGTM: Improved structure for price content with themingThe changes to the price content styling are well-implemented:
- The
themeColorfunction is correctly used to apply the theme-based color.- Moving the styling to a separate
divelement improves the structure and readability of the code.These changes enhance the consistency of the theming approach and the overall code organization.
jamiehenson
force-pushed
the
reduce-tw-class-generation
branch
from
4a95c2b to
9aa44f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/core/styles/colors/utils.ts (1)
8-10: Enhance error message for invalid class format.The current error message could be more informative to aid in debugging.
Consider updating the error message to include the invalid class name:
- throw new Error("Invalid TW class format"); + throw new Error(`Invalid Tailwind class format: ${className}`);src/core/Pricing/PricingCards.tsx (3)
100-100: Consider improving readability of long classNameThe usage of
themeColorfor the border class is correct and includes a good fallback. However, this line is quite long and might be hard to read. Consider splitting it into multiple lines for better readability.Example:
className={` relative border ${themeColor(borderClasses(border?.color)?.border ?? "border-neutral-1100")} ${border?.style ?? ""} flex-1 px-24 py-32 flex flex-col gap-24 rounded-2xl group ${delimiter ? "@[520px]:flex-row @[920px]:flex-col" : ""} min-w-[272px] backdrop-blur `}
147-147: Consider adding a fallback color for consistencyThe
themeColorfunction is correctly used to apply theming to the price text color. However, unlike previous usages, there's no fallback color provided here. Consider adding a fallback color for consistency with other themed elements.Example:
className={`ui-text-title font-medium tracking-tight leading-none ${themeColor("text-neutral-000" ?? "text-neutral-000")}`}
151-153: Consider adding a fallback color for consistencyThe
themeColorfunction is correctly used to apply theming to the price content text color. However, similar to the previous instance, there's no fallback color provided. Consider adding a fallback color for consistency with other themed elements.Example:
className={`ui-text-p3 ${themeColor("text-neutral-000" ?? "text-neutral-000")}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- .gitignore (1 hunks)
- package.json (1 hunks)
- scripts/compute-colors.ts (1 hunks)
- src/core/Icon/EncapsulatedIcon.tsx (2 hunks)
- src/core/Pricing/PricingCards.tsx (11 hunks)
- src/core/ProductTile.tsx (4 hunks)
- src/core/Tooltip.tsx (4 hunks)
- src/core/hooks/useTheming.tsx (1 hunks)
- src/core/styles/colors/Colors.stories.tsx (6 hunks)
- src/core/styles/colors/types.ts (2 hunks)
- src/core/styles/colors/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- package.json
- src/core/Icon/EncapsulatedIcon.tsx
- src/core/ProductTile.tsx
- src/core/Tooltip.tsx
- src/core/hooks/useTheming.tsx
- src/core/styles/colors/Colors.stories.tsx
- src/core/styles/colors/types.ts
🧰 Additional context used
📓 Learnings (1)
scripts/compute-colors.ts (2)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:37-37 Timestamp: 2024-10-14T14:50:06.863Z Learning: In `scripts/compute-colors.ts`, when searching for `themeColor` usages, only `.tsx` files need to be processed; there is no need to include other file types such as `.ts`, `.jsx`, or `.js`.Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:40-42 Timestamp: 2024-10-14T14:51:08.699Z Learning: In `scripts/compute-colors.ts`, when constructing the regex pattern to match `themeColor` usages, the missing separators between `joinedVariants` and `joinedPrefixes` are intentional.
🪛 Biome
src/core/styles/colors/utils.ts
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (20)
src/core/styles/colors/utils.ts (3)
1-1: LGTM: Import changes align with PR objectives.The updated imports reflect the shift from using computed colors to color group lengths, which aligns with the PR's goal of reducing generated Tailwind classes.
21-27: LGTM: Flipped variant calculation and class construction.The logic for calculating the flipped variant and constructing the new class name looks correct and aligns with the PR objectives.
1-27: Summary: Changes align with PR objectives, with minor suggestions for improvement.The modifications in this file, particularly the introduction of the
invertTailwindClassVariantfunction, align well with the PR objectives of improving theming capabilities and reducing generated Tailwind classes. The function provides a standardized approach to inverting color variants, which should contribute to more efficient dynamic theming.A few minor improvements have been suggested:
- Enhancing error messages for better debugging.
- Improving the robustness of variant parsing.
- Using safer NaN checking as per static analysis recommendations.
These changes, once implemented, will further improve the reliability and maintainability of the theming utilities.
To ensure that the new
invertTailwindClassVariantfunction is being used correctly throughout the codebase, consider running the following verification script:This script will help identify:
- Where the new function is being used.
- If there are any lingering uses of the old
determineThemeColorfunction.- Potential spots where
invertTailwindClassVariantmight be needed but not yet implemented.✅ Verification successful
All concerns from the review have been successfully addressed:
- The
invertTailwindClassVariantfunction is actively used inscripts/compute-colors.tsandsrc/core/hooks/useTheming.tsx, ensuring it aligns with the PR objectives.- There are no remaining usages of the old
determineThemeColorfunction, confirming its successful removal.No issues were found in the codebase regarding the integration and usage of these functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of invertTailwindClassVariant function # Search for usage of invertTailwindClassVariant echo "Searching for usage of invertTailwindClassVariant:" rg --type typescript --type javascript "invertTailwindClassVariant\(" -C 3 # Check for any remaining usage of the old determineThemeColor function echo "\nChecking for any remaining usage of determineThemeColor:" rg --type typescript --type javascript "determineThemeColor\(" -C 3 # Look for potential places where invertTailwindClassVariant might be needed echo "\nSearching for potential places where invertTailwindClassVariant might be needed:" rg --type typescript --type javascript "(bg|text|from|to)-\w+-\d{3}" -C 3Length of output: 740
Script:
#!/bin/bash # Description: Verify the usage of invertTailwindClassVariant and determineThemeColor functions # Search for usage of invertTailwindClassVariant in TypeScript and JavaScript files echo "Searching for usage of invertTailwindClassVariant:" rg "invertTailwindClassVariant\(" --glob "*.ts" --glob "*.tsx" -C 3 # Check for any remaining usage of the old determineThemeColor function in TypeScript and JavaScript files echo "\nChecking for any remaining usage of determineThemeColor:" rg "determineThemeColor\(" --glob "*.ts" --glob "*.tsx" -C 3 # Look for potential places where invertTailwindClassVariant might be needed by searching for Tailwind classes echo "\nSearching for potential places where invertTailwindClassVariant might be needed:" rg "(bg|text|from|to)-\w+-\d{3}" --glob "*.ts" --glob "*.tsx" -C 3Length of output: 44331
🧰 Tools
🪛 Biome
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
scripts/compute-colors.ts (8)
3-4: LGTM: Imports are appropriate for the script's functionalityThe imports look good. The
invertTailwindClassVariantfunction is imported from the utils file, which aligns with the new functionality. The color-related constants are imported from a centralized types file, promoting consistency across the codebase.
6-11: LGTM: Path definitions are clear and cross-platform compatibleThe
directoryPathandoutputPathare well-defined usingpath.join, ensuring cross-platform compatibility. The output path correctly points to thecomputed-colors.jsonfile in the appropriate directory.
13-15: LGTM: Joined constants improve maintainabilityThe joined constants for variants, prefixes, and colors are well-prepared for use in the regex pattern. This approach promotes maintainability by centralizing these values and making them easy to update if needed.
17-63: LGTM: findStringInFiles function is well-implementedThe
findStringInFilesfunction is well-structured and effectively achieves its purpose:
- It recursively searches for theme color strings in
.tsxfiles, aligning with the provided learning.- Appropriate error handling is implemented for file system operations.
- The regex pattern construction looks correct, including the intentional lack of separators between
joinedVariantsandjoinedPrefixes.The overall implementation is robust and should reliably find the required theme color strings.
66-69: LGTM: Match processing is concise and effectiveThe processing of matches is well-implemented:
- It calls
findStringInFilesto get the initial matches.- It then uses
invertTailwindClassVariantto flip each match, which aligns with the new functionality of the script.This approach effectively prepares the data for output.
72-80: LGTM: File writing logic is robust and includes necessary checksThe file writing logic is well-implemented:
- It checks if the output directory exists and creates it if necessary, addressing a potential issue raised in a previous review.
- The flipped matches are written to the JSON file using
fs.writeFileSync.- A success message is logged upon completion.
This implementation ensures that the output file is created reliably.
81-82: LGTM: Error handling is concise and informativeThe error handling for the file writing operation is well-implemented:
- It catches any errors that might occur during the file writing process.
- The error message includes the error object, which will provide detailed information for debugging if an issue occurs.
This implementation addresses a suggestion from a previous review and improves the script's robustness.
1-82: LGTM: Excellent refactoring of the compute-colors scriptThis refactoring of the
compute-colors.tsscript is well-executed and aligns perfectly with the PR objectives:
- It implements a new approach for finding and processing theme color strings, which should help reduce generated Tailwind classes.
- The script is well-structured, with clear separation of concerns between finding color strings, processing them, and writing the output.
- Robust error handling has been implemented throughout, addressing previous review comments.
- The use of centralized color-related constants and the
invertTailwindClassVariantfunction promotes consistency and maintainability.Overall, this implementation should effectively support the new dynamic theming approach while optimizing the build process.
src/core/Pricing/PricingCards.tsx (9)
9-9: LGTM: New useTheming hook importedThe import of the new
useTheminghook aligns with the PR objective of introducing standardized theming functionality.
48-51: Verify baseTheme setting in useThemingThe usage of the
useTheminghook looks good and aligns with the PR objectives. However, please confirm if settingbaseTheme: "dark"is the intended default for all cases.Can you confirm if "dark" should always be the baseTheme, or if this should be configurable?
62-62: LGTM: Correct usage of themeColor for IconThe
themeColorfunction is correctly used to apply theming to the Icon component's text color.
113-113: LGTM: Correct usage of themeColor for backgroundThe
themeColorfunction is correctly used to apply theming to the background color and hover state. The conditional application of hover styles based on thedelimiterprop is properly maintained.
121-121: LGTM: Correct usage of themeColor for text colorThe
themeColorfunction is correctly used to apply theming to the text color with a proper fallback to "text-neutral-000".
135-135: LGTM: Correct usage of themeColor for description text colorThe
themeColorfunction is correctly used to apply theming to the description text color with a proper fallback to "text-neutral-000".
160-160: Verify hover state styling in FeaturedLinkThe
themeColorfunction is correctly used to apply theming to the background and text color of the FeaturedLink component. However, the hover state (hover:text-neutral-000) is not usingthemeColor. Please verify if this is intentional or if it should also use the theming function for consistency.Is it intentional that the hover state text color is not using the
themeColorfunction?
170-172: LGTM: Correct usage of themeColor for horizontal ruleThe
themeColorfunction is correctly used to apply theming to the horizontal rule's border color. This implementation is consistent with other theming changes in the file.
Line range hint
1-253: Overall assessment: Well-implemented theming changesThe implementation of the new
useTheminghook and the use of thethemeColorfunction throughout thePricingCardscomponent is well done and aligns with the PR objectives. The changes consistently apply the new theming approach across various elements of the component.A few minor suggestions for improvement:
- Consider adding fallback colors where they're missing for consistency.
- Verify the intentionality of non-themed hover states.
- Consider improving readability of long className strings.
These minor adjustments would further enhance the consistency and maintainability of the theming implementation.
jamiehenson
force-pushed
the
reduce-tw-class-generation
branch
from
9aa44f5 to
38d950c
Compare
@kennethkalmer Right now, only |
|
If the question though was, "how does this work in |
jamiehenson
force-pushed
the
reduce-tw-class-generation
branch
from
38d950c to
0a4107b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
src/core/styles/colors/utils.ts (1)
6-11: Enhance error message for invalid class format.The error handling for invalid class name format is a good practice. However, the error message could be more informative about the expected format.
Consider updating the error message to provide more context:
- throw new Error("Invalid TW class format"); + throw new Error("Invalid Tailwind class format. Expected format: 'property-color-variant' (e.g., 'bg-blue-500')");This change will make it easier for developers to understand and correct the issue when it occurs.
src/core/ProductTile.tsx (5)
22-25: LGTM: Effective implementation of useTheming hookThe
useTheminghook is well-implemented, providing dynamic theming based on theselectedprop. This aligns perfectly with the PR objectives of enhancing theming capabilities.Consider destructuring the
selectedprop at the function parameter level for improved readability:const ProductTile = ({ name, selected, currentPage, className, onClick, }: ProductTileProps) => { const { themeColor } = useTheming({ baseTheme: "dark", theme: selected ? "light" : "dark", }); // ... };This minor change would make the code slightly more concise and easier to read.
30-30: Consider refactoring the className for improved readabilityThe usage of
themeColorfunction is correct and aligns with the new theming approach. However, theclassNamestring is becoming quite long and complex. Consider refactoring it for improved readability and maintainability.Here's a suggestion using template literals and line breaks:
className={` rounded-lg p-12 flex flex-col gap-8 transition-colors ${themeColor("bg-neutral-1200")} ${selected ? "" : "hover:bg-neutral-1100"} ${className ?? ""} `.trim()}This approach maintains the current functionality while making the className more readable and easier to maintain.
41-41: LGTM: Correct usage of themeColor functionThe
themeColorfunction is correctly implemented in the className string. For consistency with other parts of the component, consider using template literals:className={`${unavailable ? "ui-text-p2" : "ui-text-p3"} ${themeColor("text-neutral-300")} font-medium`}This change would maintain the current functionality while aligning the syntax with other parts of the component.
46-46: LGTM: Correct usage of themeColor, but consider refactoringThe
themeColorfunction is correctly implemented. However, the className string is becoming complex. Consider refactoring it for improved readability:className={` ui-text-p2 ${themeColor("text-neutral-000")} font-bold ${unavailable ? "" : "mt-[-3px]"} `.trim()}This refactoring maintains the current functionality while making the className more readable and easier to maintain.
66-66: LGTM: Correct usage of themeColor, but consider refactoring and renamingThe
themeColorfunction is correctly implemented. However, theadditionalCSSprop value is quite long and could be refactored for improved readability. Also, consider renamingadditionalCSStoadditionalClassNamefor consistency with React naming conventions.Suggested refactor:
additionalClassName={` ui-btn-secondary bg-transparent hover:bg-transparent w-full hover:text-neutral-1300 mt-8 text-center inline-block ${themeColor("text-neutral-000")} `.trim()}This refactoring improves readability while maintaining the current functionality. The prop rename aligns better with React conventions and clearly indicates its purpose.
scripts/compute-colors.ts (2)
16-19: Consider adjusting the regex pattern for more precise matchingThe regex construction is efficient as it's defined outside the file reading loop. However, the current pattern might lead to incorrect matches due to the lack of a separator between
joinedVariantsandjoinedPrefixes.Consider modifying the pattern to ensure correct matching:
const regex = new RegExp( `themeColor\\("(((${joinedVariants})(${joinedPrefixes}))?-(${joinedColors})-(000|[1-9]00|1[0-3]00))"\\)`, "g" );This adjustment allows for optional variants and prefixes while ensuring proper separation between components.
66-82: LGTM: Main execution flow and error handling are well-implementedThe main execution flow and error handling are well-implemented. The creation of the output directory if it doesn't exist is a good practice to prevent errors.
A minor suggestion for improvement:
Consider adding more detailed logging to provide better visibility into the process:
console.log(`Found ${matches.length} unique theme color matches.`); console.log(`Processed ${flippedMatches.length} flipped matches.`);This additional logging will provide more insight into the script's operation.
src/core/styles/colors/types.ts (2)
22-32: Approve the addition ofcolorsconstant andColorClassColorGroupstype with a suggestion.The introduction of the
colorsconstant andColorClassColorGroupstype improves code organization and type safety for color-related operations. This change is beneficial for maintaining consistency across the codebase.Consider adding "gui" and "secondary" to the
colorsconstant for completeness, as they are used in thecolorRolesobject. This would ensure all color groups are represented in one place:export const colors = [ "neutral", "orange", "blue", "yellow", "green", "violet", "pink", "gui", "secondary", ] as const;
186-194: Approve the addition ofcolorGroupLengthswith a suggestion for consistency.The introduction of
colorGroupLengthsis a useful feature that provides easy access to the number of colors in each group. This can be beneficial for various color-related operations in the codebase.For consistency, consider deriving all color group lengths from their respective color arrays:
export const colorGroupLengths = { neutral: neutralColors.length, orange: orangeColors.length, blue: blueColors.length, yellow: yellowColors.length, green: greenColors.length, violet: violetColors.length, pink: pinkColors.length, secondary: secondaryColors.length, gui: guiColors.length, };This approach ensures that all color group lengths are derived consistently and includes all color groups defined in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- .gitignore (1 hunks)
- package.json (1 hunks)
- scripts/compute-colors.ts (1 hunks)
- src/core/Icon/EncapsulatedIcon.tsx (2 hunks)
- src/core/Pricing/PricingCards.tsx (11 hunks)
- src/core/ProductTile.tsx (4 hunks)
- src/core/Tooltip.tsx (4 hunks)
- src/core/hooks/useTheming.tsx (1 hunks)
- src/core/styles/colors/Colors.stories.tsx (6 hunks)
- src/core/styles/colors/types.ts (2 hunks)
- src/core/styles/colors/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- .gitignore
- package.json
- src/core/Icon/EncapsulatedIcon.tsx
- src/core/Pricing/PricingCards.tsx
- src/core/Tooltip.tsx
- src/core/hooks/useTheming.tsx
- src/core/styles/colors/Colors.stories.tsx
🧰 Additional context used
📓 Learnings (3)
scripts/compute-colors.ts (1)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: scripts/compute-colors.ts:37-37 Timestamp: 2024-10-14T14:50:06.863Z Learning: In `scripts/compute-colors.ts`, when searching for `themeColor` usages, only `.tsx` files need to be processed; there is no need to include other file types such as `.ts`, `.jsx`, or `.js`.src/core/ProductTile.tsx (1)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: src/core/ProductTile.tsx:30-30 Timestamp: 2024-10-14T15:13:43.163Z Learning: In `src/core/ProductTile.tsx`, prefer not to use utility libraries like `clsx` for conditional class names.src/core/styles/colors/types.ts (3)
Learnt from: jamiehenson PR: ably/ably-ui#510 File: src/core/styles/colors/types.ts:179-183 Timestamp: 2024-10-14T15:40:02.150Z Learning: In `src/core/styles/colors/types.ts`, `secondary` is still present in `colorRoles` and hasn't been removed.Learnt from: jamiehenson PR: ably/ably-ui#486 File: src/core/styles/colors/types.ts:8-15 Timestamp: 2024-09-18T14:08:15.229Z Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.Learnt from: jamiehenson PR: ably/ably-ui#486 File: src/core/styles/colors/types.ts:8-15 Timestamp: 2024-10-08T15:37:26.766Z Learning: When updating `ColorClassVariants` and `ColorClassPrefixes`, only include variants and prefixes that relate to colour classes.
🪛 Biome
src/core/styles/colors/utils.ts
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (8)
src/core/styles/colors/utils.ts (2)
1-5: LGTM: Import changes and existing function.The updated import statement aligns well with the changes in the file. The
convertTailwindClassToVarfunction remains unchanged and continues to serve its purpose.
1-27: Summary: NewinvertTailwindClassVariantfunction implementationThe introduction of
invertTailwindClassVariantaligns well with the PR objectives of reducing generated Tailwind classes and improving theming capabilities. The function provides a mechanism to invert color variants, which is crucial for dynamic theming.Key points for improvement:
- Enhance error messages to provide more context about expected formats.
- Implement more robust parsing of Tailwind class names and variants.
- Improve the handling of special cases (e.g., the "neutral" color) and add explanatory comments.
- Ensure proper validation of inputs throughout the function.
These improvements will enhance the function's reliability, maintainability, and ease of use. Overall, the changes are a step in the right direction for achieving the PR's goals.
🧰 Tools
🪛 Biome
[error] 17-17: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
src/core/ProductTile.tsx (2)
5-5: LGTM: Import changes align with new theming approachThe addition of the
useTheminghook import and the removal ofColorClassanddetermineThemeColorimports align well with the PR objectives. This change reflects the shift towards a more centralized theming approach using the new hook.
Line range hint
1-78: Overall: Well-implemented theming changes with room for minor improvementsThe changes in this file successfully implement the new theming approach using the
useTheminghook. The refactoring aligns well with the PR objectives, enhancing the component's theming capabilities.Key points:
- The new
useTheminghook is correctly implemented and used throughout the component.- All instances of the old theming approach have been replaced with the new
themeColorfunction.- The changes maintain the component's functionality while improving its theming flexibility.
While the implementation is solid, there are opportunities to improve code readability and maintainability, particularly in complex className strings. Consider applying the suggested refactors to enhance the overall code quality.
Great job on successfully transitioning to the new theming system!
scripts/compute-colors.ts (2)
1-15: LGTM: Imports and constants are well-definedThe imports and constants are appropriately set up for the task. The use of
path.joinfor constructing file paths ensures cross-platform compatibility, which is a good practice.
1-82: Overall assessment: Well-structured script with room for minor optimizationsThe
compute-colors.tsscript is well-structured and effectively accomplishes its task of finding and processing theme color matches. It demonstrates good practices in error handling and file system operations. The suggestions provided in the review comments, if implemented, will further enhance its performance and robustness.Key points:
- Consider adjusting the regex pattern for more precise matching.
- Optimize regex usage in the
findStringInFilesfunction.- Add more detailed logging for better visibility into the process.
These improvements will make the script more efficient and easier to maintain.
src/core/styles/colors/types.ts (2)
179-183: Approve the renaming ofcolorNamestocolorRoles.The renaming of
colorNamestocolorRolesis a good improvement in clarity. It better describes the purpose of this constant, which is to define the roles or categories of colors used in the application.As noted in a previous learning, the
secondarycolor group is still present incolorRoles, which maintains consistency with existing code that may rely on it.
Line range hint
1-194: Overall, the changes improve color management and type safety.The modifications to
src/core/styles/colors/types.tsalign well with the PR objectives of enhancing theming capabilities and optimizing color-related code. The introduction ofcolors,ColorClassColorGroups, andcolorGroupLengthsprovides a more structured and type-safe approach to managing colors in the application.While there are minor suggestions for improvements, such as including all color groups in the
colorsconstant and ensuring consistency incolorGroupLengths, these changes represent a significant step forward in organizing and typing color-related code.
Jira Ticket Link / Motivation
There's been a few swings at getting dynamic theming right across our components, but so far things haven't been quite right in terms of optimisation and the usage of the functionality in the code - this PR aims to address things on both those fronts.
Summary of changes
Currently, the approach to generating extra Tailwind classes necessitated by dynamic theming has embedded a misunderstanding about Tailwind itself. Turns out, very large lookup tables are ingested by Tailwind wholesale, and result in very large build sizes.
This PR flips the approach by scanning the repo for uses of the
themeColorhelper (previouslyt), and keeping a record of the thematic variants of those classes only, meaning we only generate the classes we actually need instead of every possible combination.This is supported by the restructuring of the dynamic theming logic into a hook called
useTheming. The advantages of this are twofold:twas a convention but this wasn't reinforced anywhere, which is messy, particularly when you need some consistency when scanning for theme-oriented class names in the code.useThemingonly gives youthemeColorback.How do you manually test this?
It's a background change so hard to test directly. Look at Storybook and see all the colours work on theme-flipped components (i.e. PricingCards) I guess.
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
useTheming) to manage theme colors dynamically across components.Bug Fixes
determineThemeColorfunction to streamline theme color management.Documentation
DynamicThemingto reflect new usage patterns with theuseTheminghook.Chores
package.jsonto indicate a shift to a development version..gitignoreto adjust tracking behavior for specific color files.