Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/components/ColorBlock.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ export type ColorBlockProps = {
prefixCls?: string;
className?: string;
style?: React.CSSProperties;
/** Internal usage. Only used in antd ColorPicker semantic structure only */
innerClassName?: string;
/** Internal usage. Only used in antd ColorPicker semantic structure only */
innerStyle?: React.CSSProperties;
Comment on lines +9 to +12

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The comments for innerClassName and innerStyle are a bit verbose and could be more direct. The phrase "semantic structure only" is also slightly ambiguous. Consider using the @internal JSDoc tag to signify internal usage, which is a standard convention and can be picked up by documentation generation tools. A more concise comment would improve readability and maintainability.

Suggested change
/** Internal usage. Only used in antd ColorPicker semantic structure only */
innerClassName?: string;
/** Internal usage. Only used in antd ColorPicker semantic structure only */
innerStyle?: React.CSSProperties;
/** @internal For internal antd ColorPicker usage only. */
innerClassName?: string;
/** @internal For internal antd ColorPicker usage only. */
innerStyle?: React.CSSProperties;

onClick?: React.MouseEventHandler<HTMLDivElement>;
};

Expand All @@ -14,6 +18,8 @@ const ColorBlock: React.FC<ColorBlockProps> = ({
prefixCls,
className,
style,
innerClassName,
innerStyle,
onClick,
}) => {
const colorBlockCls = `${prefixCls}-color-block`;
Expand All @@ -23,7 +29,10 @@ const ColorBlock: React.FC<ColorBlockProps> = ({
style={style}
onClick={onClick}
>
<div className={`${colorBlockCls}-inner`} style={{ background: color }} />
<div
className={clsx(`${colorBlockCls}-inner`, innerClassName)}
style={{ background: color, ...innerStyle }}
/>
</div>
);
};
Expand Down
17 changes: 16 additions & 1 deletion tests/components.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { fireEvent, render } from '@testing-library/react';
import React from 'react';
import { expect } from 'vitest';
import ColorPicker, { type BaseSliderProps } from '../src';
import ColorPicker, { ColorBlock, type BaseSliderProps } from '../src';
import { defaultColor } from '../src/util';

describe('ColorPicker.Components', () => {
Expand Down Expand Up @@ -56,4 +56,19 @@ describe('ColorPicker.Components', () => {
fireEvent.click(alphaEle);
expect(alphaEle.textContent).toBe('0/100/33');
});

it('ColorBlock support innerClassName and innerStyle', () => {
const { container } = render(
<ColorBlock
prefixCls="test"
color="red"
innerClassName="my-inner-class"
innerStyle={{ color: '#903' }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test for innerStyle uses color: '#903'. The color CSS property sets the color of text, but the div being tested does not contain any text. While this test is technically correct and will pass, it doesn't meaningfully verify the visual effect of the applied style. To make the test more robust, consider using a style property that has a visible effect on a div element, such as border.

Suggested change
innerStyle={{ color: '#903' }}
innerStyle={{ border: '1px solid green' }}

/>,
);

const innerDiv = container.querySelector('.test-color-block-inner');
expect(innerDiv).toHaveClass('my-inner-class');
expect(innerDiv).toHaveStyle({ color: '#903' });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To align with the suggestion to test a more meaningful style property, please update this assertion to check for the border style instead of color.

Suggested change
expect(innerDiv).toHaveStyle({ color: '#903' });
expect(innerDiv).toHaveStyle({ border: '1px solid green' });

});
});
2 changes: 1 addition & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"skipLibCheck": true,
"esModuleInterop": true,
"module": "ESNext",
"types": ["vitest/globals"],
"types": ["vitest/globals", "@testing-library/jest-dom"],
"paths": {
"@@/*": [".dumi/tmp/*"],
"@rc-component/color-picker": ["src/index.tsx"]
Expand Down