new data utility and applied to button #921
Conversation
WalkthroughThe changes update how data attributes are managed in the Button component. The previous implementation utilized Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (9)
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/components/ui/Button/Button.tsx (2)
6-6: Fix import formatting for consistency.There are extra spaces in the import statement that should be removed for consistency with other imports in the file.
-import { createDataAttributes, mergeAttributes } from "~/core/createDataAttributes" +import { createDataAttributes, mergeAttributes } from "~/core/createDataAttributes";
20-24: Update comments to reflect the new implementation.The existing comments (lines 20-21) don't fully reflect the new implementation. The comments mention only applying accent color if present, but now both button and accent attributes are handled by the utility functions which filter out empty values.
- // apply data attribute for accent color - // apply attribute only if color is present + // Create data attributes for button properties and accent color + // Empty or undefined values will be automatically filtered out const dataAttributes = createDataAttributes("button", { variant, size }); const accentAttributes = createDataAttributes("accent", { color }); const mergedAttributes = mergeAttributes(dataAttributes, accentAttributes);src/core/createDataAttributes/index.ts (4)
1-7: Add JSDoc documentation to improve function discoverability.Adding JSDoc comments would help other developers understand the purpose and usage of this utility function.
+/** + * Creates data attributes with a specified prefix. + * Filters out undefined and empty string values from the attributes object. + * @param prefix - The prefix to add to each data attribute key + * @param attributes - Object containing attribute key-value pairs + * @returns Object with transformed data attribute keys + */ export const createDataAttributes = (prefix: string, attributes: Record<string, any>) => { return Object.fromEntries( Object.entries(attributes) .filter(([_, value]) => value !== undefined && value !== '') .map(([key, value]) => [`data-${prefix}-${key}`, value]) ); };
9-11: Add JSDoc and consider a more specific return type.Adding documentation and specifying the return type would improve usability and type safety.
+/** + * Merges multiple attribute objects into a single object. + * Later objects will override properties from earlier objects if keys collide. + * @param attributeObjects - Array of attribute objects to merge + * @returns A merged attribute object + */ -export const mergeAttributes = (...attributeObjects: Record<string, any>[]) => { +export const mergeAttributes = (...attributeObjects: Record<string, any>[]): Record<string, any> => { return Object.assign({}, ...attributeObjects); };
1-11: Consider using more specific types for improved type safety.The current implementation uses
Record<string, any>which is quite permissive. Consider using more specific types to restrict what values can be passed.You could define interfaces for your attribute objects and use generic types to improve type safety:
interface AttributeValue { [key: string]: string | number | boolean; } /** * Creates data attributes with a specified prefix. * Filters out undefined and empty string values from the attributes object. * @param prefix - The prefix to add to each data attribute key * @param attributes - Object containing attribute key-value pairs * @returns Object with transformed data attribute keys */ export const createDataAttributes = <T extends Record<string, any>>( prefix: string, attributes: T ): Record<string, string | number | boolean> => { return Object.fromEntries( Object.entries(attributes) .filter(([_, value]) => value !== undefined && value !== '') .map(([key, value]) => [`data-${prefix}-${key}`, value]) ); }; /** * Merges multiple attribute objects into a single object. * Later objects will override properties from earlier objects if keys collide. * @param attributeObjects - Array of attribute objects to merge * @returns A merged attribute object */ export const mergeAttributes = ( ...attributeObjects: Record<string, string | number | boolean>[] ): Record<string, string | number | boolean> => { return Object.assign({}, ...attributeObjects); };
4-4: Consider handling additional falsy values in the filter.The current filter excludes
undefinedand empty strings, but you might want to consider other falsy values likenullorfalsedepending on your use case.- .filter(([_, value]) => value !== undefined && value !== '') + .filter(([_, value]) => value !== undefined && value !== '' && value !== null)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/Button/Button.tsx(2 hunks)src/core/createDataAttributes/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/components/ui/Button/Button.tsx (1)
22-24: Good refactoring of data attributes handling!This is a good improvement over manually constructing data attributes. The code is now more maintainable and follows a consistent pattern for data attribute creation.
src/core/createDataAttributes/index.ts (1)
9-11:❓ Verification inconclusive
Add unit tests for these utility functions.
These utility functions are crucial for component behavior and should have unit tests to ensure they work as expected in various scenarios.
Consider creating tests for both functions to verify:
- That
createDataAttributescorrectly filters out undefined/empty values- That the data attribute keys are correctly formatted
- That
mergeAttributescorrectly merges multiple objects- That property collisions are resolved correctly
🏁 Script executed:
#!/bin/bash # Check if there are any tests for these utility functions echo "Searching for existing tests for createDataAttributes..." fd "createDataAttributes" --type file --extension spec.ts --extension test.ts fd "createDataAttributes" --type file --extension spec.tsx --extension test.tsx # Or use grep if fd is not available echo "Alternative search using grep..." find . -name "*.spec.ts" -o -name "*.test.ts" -o -name "*.spec.tsx" -o -name "*.test.tsx" | xargs grep -l "createDataAttributes" 2>/dev/nullLength of output: 484
Action: Add Unit Tests for Utility Functions
It appears that there are currently no unit tests for the utility functions in this file. To ensure that the
createDataAttributesandmergeAttributesfunctions behave correctly in all intended scenarios, please add unit tests that cover the following areas:
- Verify that
createDataAttributesfilters out undefined or empty values.- Check that data attribute keys are correctly formatted.
- Ensure that
mergeAttributescorrectly merges multiple objects.- Confirm that property collisions are resolved as expected.
Please create the appropriate test files (e.g., within a matching
.spec.tsor.test.tsfile) to cover these cases. After adding the tests, manually verify that the functions behave as intended under various conditions.
| ); | ||
| }; | ||
|
|
||
| export const mergeAttributes = (...attributeObjects: Record<string, any>[]) => { |
There was a problem hiding this comment.
call this composeAttributes
src/components/ui/Button/Button.tsx
Outdated
| if (color) { | ||
| data_attributes['data-accent-color'] = color; | ||
| } | ||
| const dataAttributes = createDataAttributes("button", { variant, size }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/core/hooks/createDataAttribute/createDataAttribute.test.tsx (3)
6-7: Remove unnecessary blank line.There's an extra blank line after the describe block opening. This violates the linting rule against padded blocks.
describe("createDataAttribute", () => { - test("attributes are created and applied", () => {🧰 Tools
🪛 ESLint
[error] 6-8: Block must not be padded by blank lines.
(padded-blocks)
8-26: Test is well structured but could benefit from more descriptive naming.The test correctly validates that attributes are properly created and applied to the Button component. However, consider using a more descriptive test name that indicates what functionality is being verified.
- test("attributes are created and applied", () => { + test("should create and apply multiple data attributes when valid values are provided", () => {
1-47: Consider adding tests for additional edge cases.While the current tests cover the basic functionality and handling of undefined/empty values, consider adding tests for these additional scenarios:
- Testing with
nullvalues- Testing with boolean values (
true/false)- Testing with numeric values (0, negative numbers)
- Testing the composition of multiple attribute objects with overlapping keys
Would you like me to provide examples of these additional test cases?
🧰 Tools
🪛 ESLint
[error] 6-8: Block must not be padded by blank lines.
(padded-blocks)
src/core/hooks/createDataAttribute/index.ts (3)
1-7: Add stronger type safety for thecreateDataAttributefunction.The current implementation uses
anyfor attribute values, which loses type safety. Consider using a more specific type or generic parameter to maintain better type safety while still allowing flexibility.-export const createDataAttribute = (prefix: string, attributes: Record<string, any>) => { +export const createDataAttribute = <T extends Record<string, string | number | boolean | undefined>>(prefix: string, attributes: T) => { return Object.fromEntries( Object.entries(attributes) .filter(([_, value]) => value !== undefined && value !== '') .map(([key, value]) => [`data-${prefix}-${key}`, value]) ); };
9-11: Add stronger type safety for thecomposeAttributesfunction.Similar to the previous comment, consider improving the type safety of this function by using a more specific type or generic parameter.
-export const composeAttributes = (...attributeObjects: Record<string, any>[]) => { +export const composeAttributes = <T extends Record<string, string | number | boolean>>(...attributeObjects: T[]) => { return Object.assign({}, ...attributeObjects); };
1-11: Add JSDoc comments for better documentation.Adding JSDoc comments would improve the documentation of these utility functions, making it easier for other developers to understand their purpose and usage.
+/** + * Creates data attributes with the specified prefix. + * @param prefix The prefix to add to each data attribute name. + * @param attributes Record of attribute key-value pairs. + * @returns Record of data attributes with keys formatted as `data-${prefix}-${key}`. + */ export const createDataAttribute = (prefix: string, attributes: Record<string, any>) => { return Object.fromEntries( Object.entries(attributes) .filter(([_, value]) => value !== undefined && value !== '') .map(([key, value]) => [`data-${prefix}-${key}`, value]) ); }; +/** + * Composes multiple attribute objects into a single object. + * @param attributeObjects Array of attribute objects to compose. + * @returns Single object containing all attributes from the input objects. + */ export const composeAttributes = (...attributeObjects: Record<string, any>[]) => { return Object.assign({}, ...attributeObjects); };src/components/ui/Button/Button.tsx (1)
22-30: Consider moving data attribute creation to a custom hook.Since these data attribute utilities are in a hooks directory, consider creating a custom hook that encapsulates this logic for better reusability across components.
For example:
// In a new file: src/core/hooks/createDataAttribute/useComponentAttributes.ts import { createDataAttribute, composeAttributes } from './index'; export const useButtonAttributes = (variant?: string, size?: string, color?: string) => { const dataAttributes = createDataAttribute("button", { variant, size }); const accentAttributes = createDataAttribute("accent", { color }); return composeAttributes(dataAttributes, accentAttributes); };Then in Button.tsx:
-import { createDataAttribute, composeAttributes } from "~/core/hooks/createDataAttribute" +import { useButtonAttributes } from "~/core/hooks/createDataAttribute/useComponentAttributes" // ... const Button = ({ children, type = 'button', customRootClass = '', className = '', variant = '', size = '', color = '', ...props }: ButtonProps) => { const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); - const dataAttributes = createDataAttribute("button", { variant, size }); - const accentAttributes = createDataAttribute("accent", { color }); - const composedAttributes = composeAttributes(dataAttributes, accentAttributes); + const composedAttributes = useButtonAttributes(variant, size, color); // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Button/Button.tsx(2 hunks)src/core/hooks/createDataAttribute/createDataAttribute.test.tsx(1 hunks)src/core/hooks/createDataAttribute/index.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/core/hooks/createDataAttribute/createDataAttribute.test.tsx
[error] 6-8: Block must not be padded by blank lines.
(padded-blocks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
src/core/hooks/createDataAttribute/createDataAttribute.test.tsx (2)
1-4: Imports are well organized.The imports section is clean and properly imports all necessary dependencies: the custom hooks being tested, React, testing utilities, and the Button component.
28-46: Effective testing of edge cases.This test effectively verifies that undefined or empty values are properly filtered out when creating data attributes. The assertions correctly check that the corresponding attributes are not present in the rendered button.
src/components/ui/Button/Button.tsx (3)
6-6: Well-structured import of the new utility functions.The import is correctly formatted and placed appropriately with other imports.
22-24: Good implementation of the new data attribute utilities.The code effectively uses the new utility functions to replace the manual construction of data attributes. This approach is more maintainable and reduces the risk of errors.
30-31: Ensure proper attribute spreading order.The order of spreading attributes is important. The current implementation correctly places
composedAttributesbeforeprops, which allows any data attributes directly passed inpropsto override those created by the utility functions if necessary.
| render(<Component />); | ||
|
|
||
| expect(screen.getByText("Click me")).toBeInTheDocument(); | ||
| expect(screen.getByText("Click me")).toHaveAttribute("data-button-variant","primary"); |
| @@ -0,0 +1,11 @@ | |||
| export const createDataAttribute = (prefix: string, attributes: Record<string, any>) => { | |||
There was a problem hiding this comment.
can we convert these to react hooks?
|
Closing this in favor of #934 |
closes #919
Summary by CodeRabbit
Refactor
Tests