Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(checkbox): v14 #2802

Closed
wants to merge 4 commits into from
Closed
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
1 change: 1 addition & 0 deletions scripts/build-taro.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ async function buildES(p) {
[
'src/packages/**/*.taro.{ts,tsx}',
'src/packages/**/types.ts',
'src/packages/**/icon.tsx',
'src/packages/**/context.ts',
'src/packages/**/utils.ts',
'src/utils/**/*.{ts,tsx}',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,9 @@ exports[`should match snapshot 1`] = `
<div
class="nut-checkbox nut-checkbox-reverse"
>
<svg
aria-labelledby="Checked"
class="nut-icon nut-icon-Checked nut-checkbox-icon nut-checkbox-icon-checked"
role="presentation"
viewBox="0 0 1024 1024"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M512 0C230.4 0 0 230.4 0 512s230.4 512 512 512 512-230.4 512-512S793.6 0 512 0m281.6 369.78c-14.22 11.38-193.42 130.84-321.42 321.42 0 0 0 2.84-2.85 2.84-8.53 5.69-48.35 36.98-88.17-8.53-39.82-51.2-62.58-99.56-142.23-142.22-2.84 0-2.84-2.85-2.84-2.85-8.53-11.38-39.82-56.89 19.91-56.88 45.51 0 91.02 11.38 162.13 73.95 5.69 5.69 14.22 5.69 17.07 0 34.13-39.82 173.51-190.58 332.8-238.93 0 0 19.91-2.85 31.29 14.22 5.69 11.38 11.38 22.76-5.69 36.98"
fill="currentColor"
fill-opacity="0.9"
/>
</svg>
<div
class="nut-checkbox-self nut-checkbox-self-checked"
/>
<span
class="nut-checkbox-label "
>
Expand Down
22 changes: 10 additions & 12 deletions src/packages/checkbox/__test__/checkbox.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ test('should fireEvent correctly', () => {

fireEvent.click(getByTestId('checkbox3'))
fireEvent.click(getByTestId('checkbox1'))
const icons = container.querySelectorAll('.nut-checkbox-icon-checked')
const icons = container.querySelectorAll('.nut-checkbox-self-checked')
expect(icons.length).toBe(1)
expect(limit).toBeCalledWith('min')

Expand Down Expand Up @@ -117,28 +117,26 @@ test('Render checkboxs by configure indeterminate', () => {
const { container } = render(
<Checkbox value="1" checked label="labe1" indeterminate />
)
expect(
container.querySelector('.nut-checkbox-icon-indeterminate')
).toBeTruthy()
expect(container.querySelector('.nut-checkbox-self-disabled')).toBeTruthy()
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

测试用例与测试名称不符

该测试用例命名为测试 indeterminate 状态,但实际上在验证 disabled 状态。建议修改测试用例以确保正确验证 indeterminate 状态。

建议修改为:

-  expect(container.querySelector('.nut-checkbox-self-disabled')).toBeTruthy()
+  expect(container.querySelector('.nut-checkbox-self-indeterminate')).toBeTruthy()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect(container.querySelector('.nut-checkbox-self-disabled')).toBeTruthy()
expect(container.querySelector('.nut-checkbox-self-indeterminate')).toBeTruthy()

})

test('Render checkboxs by configure disabled', () => {
const { container } = render(
<Checkbox value="1" checked label="labe1" disabled />
)
expect(container.querySelector('.nut-checkbox-icon-disabled')).toBeTruthy()
expect(
container.querySelector('.nut-checkbox-self-checked-disabled')
).toBeTruthy()
})

test('Render checkboxs by configure disabled and indeterminate', () => {
const { container } = render(
<Checkbox value="1" checked label="labe1" disabled indeterminate />
)
expect(
container.querySelector('.nut-checkbox-icon-indeterminate')
).toBeTruthy()
expect(
container.querySelector('.nut-checkbox-icon-indeterminate')
).toHaveClass('nut-checkbox-icon-disabled')
expect(container.querySelector('.nut-checkbox-self-disabled')).toBeTruthy()
// expect(
// container.querySelector('.nut-checkbox-self-indeterminate')
// ).toHaveClass('nut-checkbox-self-disabled')
Comment on lines +136 to +139
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议完善组合状态的测试覆盖

当前测试用例只验证了禁用状态,但根据测试名称,应该同时验证 disabledindeterminate 两种状态。建议取消注释并完善测试断言。

建议修改为:

  expect(container.querySelector('.nut-checkbox-self-disabled')).toBeTruthy()
-  // expect(
-  //   container.querySelector('.nut-checkbox-self-indeterminate')
-  // ).toHaveClass('nut-checkbox-self-disabled')
+  expect(container.querySelector('.nut-checkbox-self-indeterminate')).toBeTruthy()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect(container.querySelector('.nut-checkbox-self-disabled')).toBeTruthy()
// expect(
// container.querySelector('.nut-checkbox-self-indeterminate')
// ).toHaveClass('nut-checkbox-self-disabled')
expect(container.querySelector('.nut-checkbox-self-disabled')).toBeTruthy()
expect(container.querySelector('.nut-checkbox-self-indeterminate')).toBeTruthy()

})

test('list model should fireEvent correctly', () => {
Expand Down Expand Up @@ -181,7 +179,7 @@ test('list model should fireEvent correctly', () => {

fireEvent.click(getByTestId('checkbox3'))
fireEvent.click(getByTestId('checkbox1'))
const icons = container.querySelectorAll('.nut-checkbox-icon-checked')
const icons = container.querySelectorAll('.nut-checkbox-self-checked')
expect(icons.length).toBe(1)
expect(limit).toBeCalledWith('min')

Expand Down
84 changes: 60 additions & 24 deletions src/packages/checkbox/checkbox.scss
Original file line number Diff line number Diff line change
@@ -1,46 +1,82 @@
@import '../checkboxgroup/checkboxgroup.scss';

.nut-checkbox {
display: flex;
align-items: center;
.nut-checkbox-self {
width: 16px;
height: 16px;
flex-shrink: 0;
position: relative;
box-sizing: border-box;
border-radius: 50%;

&-normal {
border: 0.5px solid $color-text-disabled;
background: $color-background-overlay;
}

&-icon {
color: $color-text-disabled;
font-size: $checkbox-icon-font-size;
&-checked,
&-disabled,
&-checked-disabled {
&:after {
display: block;
position: absolute;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
content: ' ';
background: $color-primary-text;
}
}

&-icon-checked {
color: $color-primary;
transition-duration: 0.3s;
transition-property: color, border-color, background-color;
&-checked {
background: linear-gradient(
90deg,
$color-primary-stop-1 0%,
$color-primary-stop-2 100%
);
box-shadow: 0px 2px 4px 0px rgba(255, 15, 35, 0.2);

&:after {
width: 8px;
height: 8px;
border-radius: 50%;
}
}

&-disabled {
background: $color-text-disabled;

&.nut-checkbox-icon-disabled {
color: $color-primary-disabled-special;
&:after {
width: 8px;
height: 1.5px;
}
}

&-checked-disabled {
background: $color-primary-disabled-special;

&:after {
width: 8px;
height: 8px;
border-radius: 50%;
}
}
Comment on lines +16 to +62
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议将固定数值抽取为 CSS 变量

代码中存在多个魔法数字(如 8px1.5px 等),建议将这些值抽取为 CSS 变量以提高可维护性和一致性。

+ $checkbox-indicator-size: 8px;
+ $checkbox-disabled-height: 1.5px;

  &-checked {
    &:after {
-      width: 8px;
-      height: 8px;
+      width: $checkbox-indicator-size;
+      height: $checkbox-indicator-size;
      border-radius: 50%;
    }
  }

Committable suggestion skipped: line range outside the PR's diff.

}

.nut-checkbox {
display: flex;
align-items: center;

&-label {
margin-left: $checkbox-label-margin-left;
font-size: $checkbox-label-font-size;
color: $checkbox-label-color;
white-space: nowrap;

Comment on lines 72 to 74
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

建议使用文本截断 mixin 替代单独的 white-space: nowrap

代码库中已有成熟的文本截断处理方案,建议使用 @include text-ellipsis()@include oneline-ellipsis() 来处理标签文本。其他组件(如 Tabs、NoticeBar)在使用 white-space: nowrap 时都配合了文本截断样式。

建议修改为:

.nut-checkbox-label {
    margin-left: $checkbox-label-margin-left;
    font-size: $checkbox-label-font-size;
    color: $checkbox-label-color;
    @include text-ellipsis();
}
🔗 Analysis chain

建议重新考虑 white-space: nowrap 的使用

强制不换行可能导致长文本标签溢出容器。建议添加文本截断或允许适当换行的样式。

.nut-checkbox-label {
    margin-left: $checkbox-label-margin-left;
    font-size: $checkbox-label-font-size;
    color: $checkbox-label-color;
-    white-space: nowrap;
+    overflow: hidden;
+    text-overflow: ellipsis;
+    word-break: break-word;
}
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 检查是否有其他组件使用类似的标签处理方式
rg -l 'white-space:\s*nowrap' 'src/packages/*/*.scss'

Length of output: 117


Script:

#!/bin/bash
# 使用 fd 查找所有 scss 文件并使用 rg 搜索 white-space: nowrap
fd -e scss -x rg -l 'white-space:\s*nowrap' {}

Length of output: 525


Script:

#!/bin/bash
# 检查其他组件中 white-space: nowrap 的具体使用场景和相关样式
rg 'white-space:\s*nowrap' -B 2 -A 2 ./src/packages/tour/tour.scss ./src/packages/tabs/tabs.scss ./src/packages/noticebar/noticebar.scss ./src/packages/navbar/navbar.scss

# 检查是否有文本截断相关的 mixin 定义
cat ./src/styles/mixins/text-ellipsis.scss

Length of output: 2322

&-disabled {
color: $color-text-disabled;
}
}

&-icon-indeterminate {
color: $color-primary;

&.nut-checkbox-icon-disabled {
color: $color-primary-disabled-special;
}
}

&-icon-disabled {
color: $color-text-disabled;
}

&-reverse {
flex-direction: row-reverse;

Expand Down
14 changes: 7 additions & 7 deletions src/packages/checkbox/checkbox.taro.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import React, { ReactNode, useContext, useEffect, useState, FC } from 'react'
import { CheckDisabled, Checked, CheckNormal } from '@nutui/icons-react-taro'
import classNames from 'classnames'
import { View } from '@tarojs/components'
import { BasicComponent, ComponentDefaults } from '@/utils/typings'
import Context from '../checkboxgroup/context'
import { usePropsValue } from '@/utils/use-props-value'
import { CheckboxLabelPosition } from '@/packages/checkboxgroup/types'
import Icon from '@/packages/checkbox/icon'

export type CheckboxShape = 'button' | 'round'

Expand Down Expand Up @@ -97,31 +97,31 @@ export const Checkbox: FC<
const renderIcon = () => {
if (innerDisabled) {
if (innerIndeterminate) {
return <CheckDisabled className={color()} />
return <Icon tag={View} name="disabled" />
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议区分不同的禁用状态图标

目前在不同的禁用状态下使用了相同的图标名称 "disabled",这可能会导致用户体验问题。建议为不同的禁用状态使用独特的图标。

建议修改如下:

- return <Icon tag={View} name="disabled" />
+ return <Icon tag={View} name="indeterminate-disabled" />
- return <Icon tag={View} name="disabled" />
+ return <Icon tag={View} name="unchecked-disabled" />

Also applies to: 105-105

}
if (innerChecked) {
return <Checked className={color()} />
return <Icon tag={View} name="checked-disabled" />
}
return <CheckDisabled className={color()} />
return <Icon tag={View} name="disabled" />
}
if (!innerChecked) {
return React.isValidElement(icon) ? (
icon
) : (
<CheckNormal className={color()} />
<Icon tag={View} name="normal" />
)
}
if (innerIndeterminate) {
return React.isValidElement(indeterminateIcon) ? (
indeterminateIcon
) : (
<CheckDisabled className={color()} />
<Icon tag={View} name="disabled" />
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

修复 indeterminate 状态下的图标显示

当前 indeterminate 状态使用了 "disabled" 图标,这似乎是不正确的。

建议修改如下:

- return <Icon tag={View} name="disabled" />
+ return <Icon tag={View} name="indeterminate" />

Committable suggestion skipped: line range outside the PR's diff.

)
}
return React.isValidElement(activeIcon) ? (
activeIcon
) : (
<Checked className={color()} />
<Icon tag={View} name="checked" />
Comment on lines +100 to +124
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

建议添加图标名称的类型检查

为了提高代码的可维护性和类型安全性,建议为 Icon 组件的 name 属性添加类型定义。

建议在 Icon 组件中添加以下类型定义:

type IconName = 'disabled' | 'checked-disabled' | 'normal' | 'checked' | 'indeterminate' | 'indeterminate-disabled' | 'unchecked-disabled';

interface IconProps {
  tag: typeof View;
  name: IconName;
}

)
}
const color = () => {
Expand Down
37 changes: 7 additions & 30 deletions src/packages/checkbox/checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import React, {
useEffect,
useState,
} from 'react'
import { CheckDisabled, Checked, CheckNormal } from '@nutui/icons-react'
import classNames from 'classnames'
import { BasicComponent, ComponentDefaults } from '@/utils/typings'
import Context from '../checkboxgroup/context'
import { usePropsValue } from '@/utils/use-props-value'
import { CheckboxLabelPosition } from '@/packages/checkboxgroup/types'
import Icon from '@/packages/checkbox/icon'

export type CheckboxShape = 'button' | 'round'

Expand Down Expand Up @@ -102,52 +102,29 @@ export const Checkbox: FunctionComponent<
const renderIcon = () => {
if (innerDisabled) {
if (innerIndeterminate) {
return <CheckDisabled className={color()} />
return <Icon name="disabled" />
}
if (innerChecked) {
return <Checked className={color()} />
return <Icon name="checked-disabled" />
}
return <CheckDisabled className={color()} />
return <Icon name="disabled" />
}
if (!innerChecked) {
return React.isValidElement(icon) ? (
icon
) : (
<CheckNormal className={color()} />
)
return React.isValidElement(icon) ? icon : <Icon name="normal" />
}
if (innerIndeterminate) {
return React.isValidElement(indeterminateIcon) ? (
indeterminateIcon
) : (
<CheckDisabled className={color()} />
<Icon name="disabled" />
)
}
return React.isValidElement(activeIcon) ? (
activeIcon
) : (
<Checked className={color()} />
<Icon name="checked" />
)
}
const color = () => {
const cls = `${classPrefix}-icon `
if (innerDisabled) {
if (innerChecked && !innerIndeterminate) {
return `${cls}${classPrefix}-icon-checked ${classPrefix}-icon-disabled`
}
if (innerChecked && innerIndeterminate) {
return `${cls}${classPrefix}-icon-indeterminate ${classPrefix}-icon-disabled`
}
return `${cls}${classPrefix}-icon-disabled`
}
if (innerChecked) {
if (innerIndeterminate) {
return `${cls}${classPrefix}-icon-indeterminate`
}
return `${cls}${classPrefix}-icon-checked`
}
return cls
}
const renderLabel = () => {
return (
<span
Expand Down
24 changes: 24 additions & 0 deletions src/packages/checkbox/icon.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { createElement } from 'react'
import { mergeProps } from '@/utils/merge-props'

interface IconProps {
tag: any
classPrefix: string
name: 'normal' | 'disabled' | 'checked' | 'checked-disabled'
}
Alex-huxiyang marked this conversation as resolved.
Show resolved Hide resolved

const Icon = (props: Partial<IconProps>) => {
const { tag, classPrefix, name } = mergeProps(
{
tag: 'div',
classPrefix: 'nut-checkbox-self',
name: 'normal',
},
props
)
return createElement<{ className: string }>(tag, {
className: `${classPrefix} ${classPrefix}-${name}`,
})
}

export default Icon
Loading