-
-
Notifications
You must be signed in to change notification settings - Fork 142
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: add disabled api for panel #543
feat: add disabled api for panel #543
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Caution Review failedThe pull request is closed. Walkthrough此拉取请求引入了对多个组件的更改,主要是为 Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
examples/panel.tsx (1)
95-95
: 新增的禁用状态面板示例很好,建议稍作改进这个新增的禁用状态的 Cascader.Panel 实例很好地展示了新功能的使用。它与 PR 的目标一致,为开发者提供了清晰的使用示例。
为了进一步提高示例的清晰度,建议添加一个注释来解释这个禁用状态的面板的用途。例如:
+ {/* 展示禁用状态的 Cascader.Panel */} <Cascader.Panel disabled options={addressOptions} />
这样可以帮助其他开发者更快地理解这个示例的目的。
tests/Panel.spec.tsx (1)
101-112
: 测试用例覆盖了禁用状态的基本功能,但可以进一步改进这个测试用例很好地验证了 Cascader.Panel 组件的禁用状态。它检查了禁用类的存在,并确保点击选项不会触发 onChange 回调。然而,我们可以通过以下方式使测试更加全面:
- 验证所有选项都是非交互式的,而不仅仅是第一个选项。
- 检查选项在悬停或点击时的视觉状态是否保持不变。
- 验证是否正确设置了无障碍性所需的 ARIA 属性。
建议扩展测试用例以涵盖这些方面,从而提高测试的全面性和可靠性。例如:
it('disabled', () => { const onChange = jest.fn(); const { container } = render( <Cascader.Panel options={options} onChange={onChange} disabled={true} />, ); expect(container.querySelector('.rc-cascader-panel-disabled')).toBeTruthy(); // 检查所有选项 const allOptions = container.querySelectorAll('.rc-cascader-menu-item'); allOptions.forEach((option) => { fireEvent.click(option); expect(onChange).not.toHaveBeenCalled(); // 检查悬停状态 fireEvent.mouseEnter(option); expect(option).not.toHaveClass('rc-cascader-menu-item-active'); // 检查 ARIA 属性 expect(option).toHaveAttribute('aria-disabled', 'true'); }); });这个扩展的测试用例将提供更全面的覆盖,确保禁用状态在各个方面都正确实现。
src/Panel.tsx (1)
72-72
: 正确解构disabled
属性从
props
中解构disabled
属性是正确的做法,这与之前在PickType
中添加disabled
相对应。这允许Panel
组件接收和使用disabled
属性。建议:为了提高代码的可读性,可以考虑为
disabled
属性添加一个默认值,例如:disabled = false,这样可以明确指出该属性的默认行为。
src/OptionList/List.tsx (1)
35-44
: 重命名 'disabled' 为 'panelDisabled' 以提高可读性将
disabled
属性重命名为panelDisabled
有助于明确其用途,提高了代码的可读性。这与之前在RawOptionListProps
类型中的更改保持一致。建议:为了保持一致性,考虑在
RawOptionListProps
类型定义中也使用panelDisabled
而不是disabled
。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- examples/panel.tsx (1 hunks)
- src/OptionList/Column.tsx (7 hunks)
- src/OptionList/List.tsx (4 hunks)
- src/Panel.tsx (3 hunks)
- tests/Panel.spec.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (14)
src/Panel.tsx (3)
38-39
: 新增disabled
属性符合需求向
PickType
类型添加disabled
属性是正确的。这个改动使得PanelProps
类型能够包含disabled
属性,从而实现了禁用整个面板区域的功能。这与 PR 的目标一致。
205-205
: 正确传递disabled
属性到RawOptionList
将
disabled
属性传递给RawOptionList
组件是正确的做法。这确保了禁用状态可以正确地应用到选项列表中。建议:请确保
RawOptionList
组件已正确实现了对disabled
属性的处理。可以通过以下步骤进行验证:
- 检查
RawOptionList
组件的实现,确保它正确使用了disabled
属性。- 添加或更新相关的单元测试,以验证
disabled
属性在RawOptionList
中的行为。- 进行手动测试,确保当
Panel
被禁用时,RawOptionList
中的所有选项都无法交互。
Line range hint
1-209
: 总结:成功实现了 Panel 的禁用功能本次更改成功地为
Panel
组件添加了disabled
属性,实现了禁用整个面板区域的功能。主要修改包括:
- 在
PickType
中添加disabled
属性。- 在
Panel
组件中解构并使用disabled
属性。- 将
disabled
属性传递给RawOptionList
组件。这些更改符合 PR 的目标,提高了 Cascader 组件的灵活性和可用性。建议进行充分的测试,特别是
RawOptionList
组件中disabled
属性的行为,以确保功能正确实现。src/OptionList/List.tsx (4)
24-31
: 新增 'disabled' 属性以支持面板禁用功能这个更改通过在
RawOptionListProps
类型中添加disabled
属性,为RawOptionList
组件引入了禁用整个面板的功能。这与 PR 的目标一致,增强了组件的灵活性。
119-123
: 更新 isSelectable 函数以支持面板级禁用
isSelectable
函数的更新确保了当面板被禁用时(panelDisabled
为 true),所有选项都不可选择。这个改动正确地实现了面板级禁用功能,与 PR 的目标一致。这个更改有效地阻止了用户在面板被禁用时与选项进行交互,提高了组件的可用性和一致性。
223-223
: 将 panelDisabled 状态传递给 Column 组件通过将
panelDisabled
添加到columnProps
对象中,确保了禁用状态可以正确地传递给Column
组件。这个更改是实现面板级禁用功能的必要步骤,与之前的修改保持一致。
241-241
: 将 panelDisabled 属性传递给 Column 组件这个更改确保了
panelDisabled
状态被正确地传递给Column
组件,这对于在整个组件层次结构中实现面板级禁用功能是必要的。这与之前的修改保持一致,完成了禁用功能的实现。建议:验证
Column
组件是否正确处理了panelDisabled
属性,以确保禁用功能在所有层级都能正常工作。src/OptionList/Column.tsx (7)
26-26
: 新增属性panelDisabled
有助于控制面板的禁用状态在
ColumnProps
接口中添加了可选属性panelDisabled?: boolean;
,实现了对整个面板禁用状态的控制,增强了组件的灵活性。
42-42
: 正确地解构了新的参数panelDisabled
在函数组件的参数列表中添加
panelDisabled
,确保组件内部可以访问并使用该属性。
59-60
: 引入isOptionDisabled
函数,统一禁用状态判断定义了
isOptionDisabled = (optionDisabled?: boolean) => panelDisabled || optionDisabled;
,统一了面板和选项的禁用状态判断逻辑,提升了代码的可读性和维护性。
122-122
: 在triggerOpenPath
中正确应用了禁用状态检查使用
isOptionDisabled(disabled)
代替原先的disabled
判断,确保当面板或选项被禁用时,不会触发打开路径的逻辑。
134-134
: 更新了选择条件,考虑了面板的禁用状态在
triggerSelect
函数中,条件更新为if (isSelectable(option) && !isOptionDisabled(disabled))
,确保只有在面板未禁用且选项可选择的情况下,才会执行选择操作。
155-155
: 调整了菜单项的禁用样式判断在类名计算中,使用
isOptionDisabled(disabled)
,确保当面板或选项被禁用时,菜单项正确应用禁用样式。
192-192
: 确保复选框在禁用状态下不可交互在复选框组件中,将
disabled
属性设置为isOptionDisabled(disabled) || disableCheckbox
,保证当面板或选项被禁用时,复选框也被禁用,防止用户进行交互。
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #543 +/- ##
=======================================
Coverage 99.83% 99.83%
=======================================
Files 23 23
Lines 611 614 +3
Branches 185 177 -8
=======================================
+ Hits 610 613 +3
Misses 1 1 ☔ View full report in Codecov by Sentry. |
src/OptionList/Column.tsx
Outdated
const isOptionDisabled = (optionDisabled?: boolean) => panelDisabled || optionDisabled; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
上次我尝试解决,也是 props 透传,感觉很丑陋。 或许可以用 context 的方式, 我也拿捏不准
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯嗯好的,我再去看一下
src/OptionList/List.tsx
Outdated
@@ -100,10 +116,11 @@ const RawOptionList = React.forwardRef<RefOptionListProps, RawOptionListProps>(( | |||
}; | |||
|
|||
const isSelectable = (option: DefaultOptionType) => { | |||
const { disabled } = option; | |||
if (panelDisabled) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
拆行
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个拆行指的是👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/panel.tsx (3 hunks)
- src/OptionList/Column.tsx (7 hunks)
- src/OptionList/List.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/OptionList/Column.tsx
- src/OptionList/List.tsx
🔇 Additional comments (1)
examples/panel.tsx (1)
Line range hint
64-82
: 状态管理和按钮实现完善!状态管理使用了 React 的
useState
Hook,切换按钮的实现简洁明了,并且按钮文本会根据状态动态更新,符合 React 最佳实践。
<Cascader.Panel | ||
value={value} | ||
options={addressOptions} | ||
onChange={nextValue => { | ||
console.log('Change:', nextValue); | ||
setValue(nextValue); | ||
}} | ||
disabled={disabled} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
建议提取共同的 Panel 属性
多个 Cascader.Panel
组件都使用了相同的 disabled
属性。建议将共同的属性提取为一个对象,以提高代码的可维护性和减少重复。
+ const commonPanelProps = {
+ disabled,
+ options: addressOptions,
+ };
return (
<>
{/* ... */}
<Cascader.Panel
- value={value}
- options={addressOptions}
- onChange={nextValue => {
- console.log('Change:', nextValue);
- setValue(nextValue);
- }}
- disabled={disabled}
+ {...commonPanelProps}
+ value={value}
+ onChange={nextValue => {
+ console.log('Change:', nextValue);
+ setValue(nextValue);
+ }}
/>
<Cascader.Panel
+ {...commonPanelProps}
checkable
value={value2}
- options={addressOptions}
- disabled={disabled}
onChange={nextValue => {
console.log('Change:', nextValue);
setValue2(nextValue);
}}
/>
- <Cascader.Panel options={addressOptions} disabled={disabled} direction="rtl" />
+ <Cascader.Panel {...commonPanelProps} direction="rtl" />
Also applies to: 101-101, 104-104
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/OptionList/Column.tsx (2)
59-59
: 函数实现简洁明了!
isOptionDisabled
函数很好地处理了全局和选项级别的禁用状态。不过建议添加 JSDoc 注释来说明函数的用途。建议添加如下注释:
+/** + * 判断选项是否被禁用 + * @param optionDisabled - 选项级别的禁用状态 + * @returns 是否禁用 + */ const isOptionDisabled = (optionDisabled?: boolean) => disabled || optionDisabled;
Line range hint
155-192
: 建议增强无障碍性支持为了提高组件的可访问性,建议:
- 添加适当的
aria-disabled
属性- 确保键盘导航在禁用状态下的正确行为
- 添加适当的颜色对比度以提高视觉可访问性
建议添加如下属性:
<li key={fullPathKey} className={classNames(menuItemPrefixCls, { [`${menuItemPrefixCls}-expand`]: !isMergedLeaf, [`${menuItemPrefixCls}-active`]: activeValue === value || activeValue === fullPathKey, [`${menuItemPrefixCls}-disabled`]: isOptionDisabled(optionDisabled), [`${menuItemPrefixCls}-loading`]: isLoading, })} style={dropdownMenuColumnStyle} role="menuitemcheckbox" title={title} aria-checked={checked} + aria-disabled={isOptionDisabled(optionDisabled)} data-path-key={fullPathKey}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/__snapshots__/search.spec.tsx.snap
is excluded by!**/*.snap
📒 Files selected for processing (1)
- src/OptionList/Column.tsx (9 hunks)
🔇 Additional comments (3)
src/OptionList/Column.tsx (3)
26-26
: 接口变更看起来不错!
disabled
属性的类型定义清晰且符合 TypeScript 最佳实践。
65-65
: 选项信息映射的修改合理!解构和属性传递的方式保持了代码的一致性,命名清晰。
Also applies to: 87-87
122-124
: 建议考虑使用 Context 优化禁用状态的传递目前的实现通过属性传递(prop drilling)来处理禁用状态,这种方式虽然可行,但可能导致代码维护性降低。考虑使用 React Context 来管理禁用状态,这样可以:
- 减少属性传递链
- 提高代码可维护性
- 使组件之间的耦合度降低
让我们看看是否有其他组件也在传递 disabled 属性:
Also applies to: 134-136, 155-155, 192-192
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/OptionList/List.tsx (2 hunks)
🔇 Additional comments (2)
src/OptionList/List.tsx (2)
24-31
: 类型定义更新正确类型定义中添加
disabled
属性的方式符合 TypeScript 最佳实践。
35-44
: 属性解构合理从 props 中解构
disabled
的方式正确,且保持了字母顺序。
if (disabled) { | ||
return false; | ||
} | ||
|
||
const { disabled: optionDisabled } = option; | ||
const isMergedLeaf = isLeaf(option, fieldNames); | ||
return !disabled && (isMergedLeaf || changeOnSelect || multiple); | ||
|
||
return !optionDisabled && (isMergedLeaf || changeOnSelect || multiple); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
建议优化 isSelectable 函数的实现
当前实现逻辑正确,但可以更加简洁。建议合并条件判断,减少代码行数。
建议如下修改:
const isSelectable = (option: DefaultOptionType) => {
- if (disabled) {
- return false;
- }
-
const { disabled: optionDisabled } = option;
const isMergedLeaf = isLeaf(option, fieldNames);
-
- return !optionDisabled && (isMergedLeaf || changeOnSelect || multiple);
+ return !disabled && !optionDisabled && (isMergedLeaf || changeOnSelect || multiple);
};
📝 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.
if (disabled) { | |
return false; | |
} | |
const { disabled: optionDisabled } = option; | |
const isMergedLeaf = isLeaf(option, fieldNames); | |
return !disabled && (isMergedLeaf || changeOnSelect || multiple); | |
return !optionDisabled && (isMergedLeaf || changeOnSelect || multiple); | |
const { disabled: optionDisabled } = option; | |
const isMergedLeaf = isLeaf(option, fieldNames); | |
return !disabled && !optionDisabled && (isMergedLeaf || changeOnSelect || multiple); |
* feat: add disabled api for panel * feat: add test case * docs: add demo * fix: test case fixed * feat: adjust demo position * feat: improve demo * chore: reorganize code for better readability * test: update snapshot * fix: lint fix * chore: adjust code structure * test: update snapshot * chore: code optimization --------- Co-authored-by: 小豪 <aojunhao@cai-inc.com>
Cascader底层组件rc-cascader的Panel模式支持传入disabled去禁用整个Panel区域
Summary by CodeRabbit
Summary by CodeRabbit
新特性
Cascader.Panel
组件中添加了disabled
属性,允许用户设置面板为非交互状态。Column
组件的功能,支持disabled
属性,以更好地管理选项的禁用状态。测试
Cascader.Panel
组件添加了新的测试用例,以验证禁用状态的行为。