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

fix(comp:table): the indent not work and selected records not cached #1484

Merged
merged 2 commits into from Mar 6, 2023

Conversation

danranVm
Copy link
Member

@danranVm danranVm commented Mar 6, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added/updated or not needed
  • Docs and demo have been added/updated or not needed

What is the current behavior?

  • 展开列叶子节点的 indent 配置不生效
  • 对于后端分页的场景,已经选中的数据,再切换页面后,没有被缓存,onChange 回调的数据中丢失。

What is the new behavior?

Other information

multiple: true,
showIndex: false,
onChange: console.log,
},
{
key: 'Name',
title: 'Name',
Copy link

Choose a reason for hiding this comment

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

with the code review.

First, I would check that the type of the column is set correctly to 'selectable'. It should be set to 'selectable' and not 'text' or any other value.

Second, I would check that the alignment is set correctly to 'center'. This will ensure that the column is aligned correctly.

Third, I would check that the multiple value is set correctly to 'true'. This will allow users to select multiple rows in the table.

Fourth, I would check that the showIndex is set correctly to 'false'. This will prevent the index being displayed when selecting multiple rows.

Fifth, I would check that the onChange value is set correctly to 'console.log'. This will log all changes made on the table.

Finally, I would check that no other columns are affected by this change. That is, any other columns should remain unchanged.

I hope this helps. Please let me know if you have any questions.

@idux-bot
Copy link

idux-bot bot commented Mar 6, 2023

This preview will be available after the AzureCI is passed.

@@ -34,7 +34,7 @@ const columns: TableColumn<Data>[] = [
{
type: 'expandable',
width: 160,
indent: 10,
indent: 16,
title: 'Event Name',
dataKey: 'eventName',
customCell: 'name',
Copy link

Choose a reason for hiding this comment

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

with the code review:

  1. The indent value for the column is changed from 10 to 16. It's better to ensure that this change does not affect other parts of the application.

  2. It's important to make sure that the dataKey of the column matches the actual data in the table. The dataKey should be accurate and consistent.

  3. The customCell should be specified correctly and should match the actual cell type. If the wrong cell type is used, it may cause problems in the application.

  4. It's good practice to add comments to explain the purpose of the changes so that future developers can easily understand the code.

@@ -85,7 +85,7 @@ export type TableColumn<T = any, V = any> =
| `type` | 列类型 | `'expandable'` | - | - | 必填 |
| `disabled` | 设置是否允许行展开 | `(record:T) => boolean` | - | - | - |
| `icon` | 展开按钮图标 | `string \| VNode \| ((data: { expanded: boolean; record: T }) => string \| VNodeChild)` | `'right'` | ✅ | - |
| `indent` | 展示树形数据时,每层缩进的宽度 | `number` | `12` | - | - |
| `indent` | 展示树形数据时,每层缩进的宽度 | `number` | - | - | - |
| `trigger` | 不通过图标,触发行展开的方式 | `'click' \| 'doubleClick'` | - | - | - |
| `onChange` | 展开状态发生变化时触发 | `(expendedRowKeys: (string \| number)[], expendedRecords: T[]) => void` | - | - | - |
| `onExpand` | 点击展开图标,或通过 `trigger` 触发 | `(expanded: boolean, record: T) => void` | - | - | - |
Copy link

Choose a reason for hiding this comment

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

the code review

The code looks good. We can see that the 'indent' property of the TableColumn type has been changed from the default '12' to '-', which means that the indentation for the tree data will now be undefined. This could potentially lead to unexpected behavior and should be tested thoroughly before deploying.

We can also suggest that the 'disabled' property should have a default value if it is not specified by the user, as this could also lead to unexpected behavior. Additionally, the function signature for 'onChange' and 'onExpand' should be checked to make sure they are properly handling the data they receive.

cacheSelectedMap.set(key, currData)
} else {
currData = cacheSelectedMap.get(key)
}
currData && selectedRecords.push(currData.record)
})
callEmit(onChange, tempRowKeys, selectedRecords)
Copy link

Choose a reason for hiding this comment

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

with the code review:

  1. The code should be properly indented and formatted for easy readability.
  2. The use of variables should be optimized.
  3. The cacheSelectedMap variable should be declared outside the emitChange function.
  4. The if-else statement should be used to check if the data exists in the dataMap.
  5. Comments should be added to explain the logic of the code.

}

return (
<button class={`${prefixCls}-expandable-trigger`} style={style} type="button" onClick={props.handleExpend}>
{iconNode}
Copy link

Choose a reason for hiding this comment

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

code review:

  1. It looks like the code is checking for disabled state before proceeding further. This is good as it provides a secure way of handling user input.
  2. The code is using the slots API to get customIcon if provided and fallback to icon if not. This is also a good practice.
  3. The code also checks if the iconRender is a function or a string and accordingly sets the icon node.
  4. The code also adds a button with class and type attributes which is a good practice.
  5. Overall, the code looks well written and secure. There are no major risks or improvements that come to mind.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #1484 (69497c6) into main (e044390) will not change coverage.
The diff coverage is n/a.

❗ Current head 69497c6 differs from pull request most recent head 1c13040. Consider uploading reports for the commit 1c13040 to get more accurate results

@@           Coverage Diff           @@
##             main    #1484   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files         331      331           
  Lines       30742    30742           
  Branches     3533     3533           
=======================================
  Hits        28511    28511           
  Misses       2231     2231           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@danranVm danranVm linked an issue Mar 6, 2023 that may be closed by this pull request
1 task
@danranVm danranVm merged commit a3d5bb7 into IDuxFE:main Mar 6, 2023
@danranVm danranVm deleted the fix-table branch March 6, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[comp:table ]树表格展示都是同级,没有体现父子关系
1 participant