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

功能: Table 支持 Cell 合并 #31

Merged
merged 6 commits into from
Jul 11, 2024
Merged

Conversation

jschyz
Copy link
Contributor

@jschyz jschyz commented Jul 9, 2024

实现参考
https://github.com/nlulic/slate-table
飞书表格

WangEditor 支持 Table Cell 合并 [预览]

基础能力

  1. 拖动选择单元格,隐藏默认选区行为
  2. 合并 / 拆分 单元格

列拖拽

  1. 表头出现单元格合并,每列可单独拖动设置宽度
  2. 合并单元格内聚焦时,不会出现拖拽引导线

行列新增 / 删除

  1. 支持单元格区间内 新增 / 删除行列功能

宽度自适应

  1. 当Table宽度100%时,拖动单元格能力禁用。
(无法判断单元格哪个该新增宽度,哪个该减小宽度)

图片自适应

  1. 图片最大适应单元格 100% 宽度,不会存在撑大行为
  2. 修改图片自身尺寸最大限制 🚫

Summary by CodeRabbit

  • New Features

    • Added the ability to delete and insert columns in tables, with improved handling of cell spans and content adjustments.
    • Introduced column width management, allowing for more flexible table rendering.
    • Added functionality for merging and splitting table cells directly from the menu.
    • Enhanced table rendering with better support for hidden cells and selection states.
  • Bug Fixes

    • Improved column resizing behavior and event handling in table interactions.
  • Refactor

    • Streamlined the logic for column operations in tables, enhancing performance and maintainability.

Copy link
Contributor

coderabbitai bot commented Jul 9, 2024

Walkthrough

The recent changes to the table-module package involve enhancements and refactoring aimed at improving table manipulation functionality. Key updates include handling table column deletion and insertion, adding new features for merging and splitting cells, and introducing column width configurations. These changes enhance table editing capabilities, improve performance, and refine rendering logic.

Changes

File(s) Change Summary
DeleteCol.ts, InsertCol.ts Refactored logic for deleting and inserting columns, including handling cells, column spans, widths, and table structures.
InsertTable.ts Introduced a columnWidths array within the TableElement object for specifying column widths.
MergeCell.ts, SplitCell.ts Added functionality for merging and splitting table cells, along with related configurations.
plugin.ts Added an import for withSelection and integrated a call to withSelection within withTable for improved selection handling.
render-cell.tsx Refactored table cell rendering logic, including removal of column width resizing and adjustments based on cell properties.
render-table.tsx Enhanced table rendering, managing cell borders, column resizing, and event listeners for table interactions.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant TableEditor
    participant DeleteCol
    participant InsertCol
    participant MergeCell
    participant SplitCell
    participant RenderTable
    participant Plugin
    
    User->>TableEditor: Delete Column
    TableEditor->>DeleteCol: Execute delete column logic
    DeleteCol-->>TableEditor: Updated table structure

    User->>TableEditor: Insert Column
    TableEditor->>InsertCol: Execute insert column logic
    InsertCol-->>TableEditor: Updated table structure

    User->>TableEditor: Merge Cells
    TableEditor->>MergeCell: Execute merge cell logic
    MergeCell-->>TableEditor: Merged cells

    User->>TableEditor: Split Cells
    TableEditor->>SplitCell: Execute split cell logic
    SplitCell-->>TableEditor: Split cells

    TableEditor->>RenderTable: Render updated table
    RenderTable-->>User: Display updated table
Loading

Poem

In the land of code where tables dwell,
Columns shift and cells now tell,
Of merges and splits, widths anew,
A dance of data, sleek and true.
With each new change, we celebrate,
A table's tale, forever great.
🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@jschyz
Copy link
Contributor Author

jschyz commented Jul 9, 2024

本地测试截图

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 25ed7e7 and 4956621.

Files selected for processing (34)
  • packages/basic-modules/src/modules/image/render-elem.tsx (8 hunks)
  • packages/core/src/editor/dom-editor.ts (1 hunks)
  • packages/core/src/text-area/syncSelection.ts (1 hunks)
  • packages/editor/src/init-default-config/config/hoverbar.ts (1 hunks)
  • packages/table-module/package.json (1 hunks)
  • packages/table-module/src/assets/index.less (3 hunks)
  • packages/table-module/src/constants/svg.ts (1 hunks)
  • packages/table-module/src/locale/en.ts (1 hunks)
  • packages/table-module/src/locale/zh-CN.ts (1 hunks)
  • packages/table-module/src/module/column-resize.ts (1 hunks)
  • packages/table-module/src/module/custom-types.ts (2 hunks)
  • packages/table-module/src/module/elem-to-html.ts (2 hunks)
  • packages/table-module/src/module/index.ts (2 hunks)
  • packages/table-module/src/module/menu/DeleteCol.ts (2 hunks)
  • packages/table-module/src/module/menu/DeleteRow.ts (2 hunks)
  • packages/table-module/src/module/menu/InsertCol.ts (2 hunks)
  • packages/table-module/src/module/menu/InsertRow.ts (2 hunks)
  • packages/table-module/src/module/menu/InsertTable.ts (2 hunks)
  • packages/table-module/src/module/menu/MergeCell.ts (1 hunks)
  • packages/table-module/src/module/menu/SplitCell.ts (1 hunks)
  • packages/table-module/src/module/menu/index.ts (2 hunks)
  • packages/table-module/src/module/plugin.ts (2 hunks)
  • packages/table-module/src/module/render-elem/render-cell.tsx (2 hunks)
  • packages/table-module/src/module/render-elem/render-table.tsx (4 hunks)
  • packages/table-module/src/module/table-cursor.ts (1 hunks)
  • packages/table-module/src/module/weak-maps.ts (1 hunks)
  • packages/table-module/src/module/with-selection.ts (1 hunks)
  • packages/table-module/src/utils/has-common.ts (1 hunks)
  • packages/table-module/src/utils/index.ts (1 hunks)
  • packages/table-module/src/utils/is-of-type.ts (1 hunks)
  • packages/table-module/src/utils/matrices.ts (1 hunks)
  • packages/table-module/src/utils/options.ts (1 hunks)
  • packages/table-module/src/utils/point.ts (1 hunks)
  • packages/table-module/src/utils/types.ts (1 hunks)
Files skipped from review due to trivial changes (3)
  • packages/core/src/text-area/syncSelection.ts
  • packages/table-module/src/module/weak-maps.ts
  • packages/table-module/src/utils/index.ts
Additional context used
Biome
packages/table-module/src/utils/point.ts

[error] 11-11: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

packages/table-module/src/module/menu/InsertCol.ts

[error] 68-68: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 57-57: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


[error] 68-68: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/with-selection.ts

[error] 16-17: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)


[error] 26-26: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)


[error] 41-41: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)


[error] 49-49: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)

packages/table-module/src/module/menu/MergeCell.ts

[error] 121-121: Unnecessary continue statement

Unsafe fix: Delete the unnecessary continue statement

(lint/correctness/noUnnecessaryContinue)

packages/table-module/src/module/menu/DeleteCol.ts

[error] 76-76: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 65-65: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


[error] 76-76: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/render-elem/render-table.tsx

[error] 142-143: Missing key property for this element in iterable.

The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.

(lint/correctness/useJsxKeyInIterable)


[error] 155-156: Missing key property for this element in iterable.

The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.

(lint/correctness/useJsxKeyInIterable)

Additional comments not posted (79)
packages/table-module/src/locale/zh-CN.ts (1)

16-17: Localization strings added for cell merging and splitting.

The added localization strings for 'merge cell' and 'split cell' are appropriate and consistent with the existing entries.

packages/table-module/src/utils/point.ts (1)

1-17: Class definition and methods look good.

The Point class is well-defined and the methods are correct.

Tools
Biome

[error] 11-11: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

packages/table-module/src/utils/types.ts (1)

1-22: Type definitions look good.

The type definitions for CellElement, WithType, NodeEntryWithContext, SelectionMode, and Edge are correct and clear.

packages/table-module/src/utils/options.ts (3)

1-3: Import statements look good.

The import statements correctly bring in necessary types from 'slate'.


5-16: Interface WithTableOptions is well-defined.

The interface WithTableOptions defines the structure for table-related block elements, which is essential for table functionalities.


18-29: Default table options are correctly set.

The DEFAULT_WITH_TABLE_OPTIONS object provides default values for table-related block elements, ensuring consistency across the application.

packages/table-module/src/utils/is-of-type.ts (3)

1-4: Import statements look good.

The import statements correctly bring in necessary modules and types.


5-7: Function isElement is well-implemented.

The function isElement correctly checks if a node is an element and includes a 'type' property.


9-17: Function isOfType is well-implemented.

The function isOfType generates a NodeMatch function to match elements of specific types, which is essential for table functionalities.

packages/table-module/src/utils/has-common.ts (2)

1-4: Import statements look good.

The import statements correctly bring in necessary modules and types.


5-26: Function hasCommon is well-implemented.

The function hasCommon correctly determines if two paths share a common ancestor node of a specific type, which is essential for table functionalities.

packages/table-module/src/module/custom-types.ts (2)

17-19: Addition of hidden property in TableCellElement is appropriate.

The hidden property is added to TableCellElement for setting the display attribute, which is essential for managing cell visibility.


31-38: Addition of new properties in TableElement is appropriate.

The new properties (scrollWidth, height, resizingIndex, isResizing, isHoverCellBorder, columnWidths) are added to TableElement for managing table layout and resizing functionalities.

packages/table-module/src/module/elem-to-html.ts (2)

12-12: Approve the addition of table-layout: fixed in tableToHtml.

The addition of table-layout: fixed ensures consistent layout for tables with merged cells.


25-29: Approve the handling of hidden cells in tableCellToHtml.

The changes correctly handle hidden cells by adding a display:none style when the hidden property is true.

packages/editor/src/init-default-config/config/hoverbar.ts (2)

36-36: Approve the addition of mergeTableCell menu key.

The addition of mergeTableCell menu key is necessary for the merge cell functionality in the hoverbar menu.


37-37: Approve the addition of splitTableCell menu key.

The addition of splitTableCell menu key is necessary for the split cell functionality in the hoverbar menu.

packages/table-module/src/module/index.ts (2)

21-21: Approve the addition of mergeTableCellConf.

The addition of mergeTableCellConf is necessary for the merge cell functionality in the table module.


22-22: Approve the addition of splitTableCellConf.

The addition of splitTableCellConf is necessary for the split cell functionality in the table module.

packages/table-module/package.json (1)

45-45: Approve the addition of lodash.debounce as a peer dependency.

The addition of lodash.debounce as a peer dependency is necessary for handling debounce functionality in the table module.

packages/table-module/src/module/menu/index.ts (3)

14-15: Imports look good.

The new imports for MergeCell and SplitCell are necessary for the new menu configurations.


74-79: New configuration looks good.

The mergeTableCellConf configuration follows the pattern of other menu configurations.


81-86: New configuration looks good.

The splitTableCellConf configuration follows the pattern of other menu configurations.

packages/table-module/src/module/render-elem/render-cell.tsx (3)

12-12: Import looks good.

The new import for TableCursor is necessary for the new logic handling cell selection.


27-36: Logic for handling hidden cells looks good.

The logic correctly handles hidden cells and selection in non-first row cells.


50-57: Logic for handling hidden cells looks good.

The logic correctly handles hidden cells and selection in first row cells.

packages/table-module/src/assets/index.less (2)

16-16: Style change looks good.

The style sets the table layout to fixed, which is necessary for handling column resizing and hidden cells.


50-88: Styles for column resizing and selection look good.

The styles correctly handle column resizing and selection highlighting.

packages/table-module/src/utils/matrices.ts (2)

1-42: Function looks good.

The matrices function correctly generates matrices for table sections.


44-88: Function looks good.

The filledMatrix function correctly generates filled matrices for table sections.

packages/table-module/src/module/table-cursor.ts (4)

18-24: LGTM!

The function isInTable correctly checks if the selection is inside a table.


30-43: LGTM!

The function selection correctly retrieves a matrix representing the selected cells within a table.


46-75: LGTM!

The function unselect correctly clears the selection from the table and the browser selection.


81-88: LGTM!

The function isSelected correctly checks if a given cell is part of the current table selection.

packages/table-module/src/module/menu/InsertRow.ts (4)

Line range hint 15-17:
LGTM!

The method getValue correctly returns an empty string as no value is needed.


Line range hint 19-21:
LGTM!

The method isActive correctly returns false as no active state is needed.


Line range hint 23-33:
LGTM!

The method isDisabled correctly checks if the insert row operation should be disabled based on the current selection.


Line range hint 35-112:
LGTM!

The method exec correctly performs the insert row operation, handling cell merging and inserting a new row at the correct position.

packages/table-module/src/module/menu/DeleteRow.ts (4)

Line range hint 13-15:
LGTM!

The method getValue correctly returns an empty string as no value is needed.


Line range hint 17-19:
LGTM!

The method isActive correctly returns false as no active state is needed.


Line range hint 21-31:
LGTM!

The method isDisabled correctly checks if the delete row operation should be disabled based on the current selection.


Line range hint 33-118:
LGTM!

The method exec correctly performs the delete row operation, handling cell merging and deleting the selected row at the correct position.

packages/table-module/src/module/menu/InsertCol.ts (4)

Line range hint 15-17:
LGTM!

The method getValue correctly returns an empty string as no value is needed.


Line range hint 19-21:
LGTM!

The method isActive correctly returns false as no active state is needed.


Line range hint 23-33:
LGTM!

The method isDisabled correctly checks if the insert column operation should be disabled based on the current selection.


Line range hint 35-123:
LGTM!

The method exec correctly performs the insert column operation, handling cell merging and inserting a new column at the correct position.

Tools
Biome

[error] 68-68: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 57-57: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


[error] 68-68: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/with-selection.ts (3)

9-13: Ensure the selection logic is correct and efficient.

The logic for handling selection operations is well-implemented, ensuring that the selection remains active when dragging cell widths. However, verify that this does not introduce any performance issues or unexpected behaviors.


52-98: Ensure the bounds calculation is accurate and efficient.

The nested loops for calculating the initial bounds and expanding the selection based on rowspan and colspan are correct. However, ensure that this logic is efficient and does not cause performance degradation for large tables.


100-117: Verify the correctness of the selection state updates.

The logic for updating the editor's selection state using EDITOR_TO_SELECTION and EDITOR_TO_SELECTION_SET appears correct. Ensure that these updates accurately reflect the user's selection and do not introduce any inconsistencies.

packages/table-module/src/module/menu/MergeCell.ts (4)

14-17: LGTM!

The getValue method correctly returns an empty string as no value needs to be retrieved.


19-22: LGTM!

The isActive method correctly returns false as there is no need to check for active state.


24-26: Ensure the canMerge method is accurate.

The isDisabled method relies on the canMerge method to determine if merging is possible. Verify the correctness of the canMerge method to ensure this logic is accurate.


121-121: Remove the unnecessary continue statement.

The continue statement is not required here and can be removed to simplify the code.

- continue

Likely invalid or redundant comment.

Tools
Biome

[error] 121-121: Unnecessary continue statement

Unsafe fix: Delete the unnecessary continue statement

(lint/correctness/noUnnecessaryContinue)

packages/table-module/src/module/menu/DeleteCol.ts (4)

Line range hint 14-17:
LGTM!

The getValue method correctly returns an empty string as no value needs to be retrieved.


Line range hint 19-22:
LGTM!

The isActive method correctly returns false as there is no need to check for active state.


Line range hint 24-36:
LGTM!

The isDisabled method correctly determines if the current selection is valid for deleting a column.


76-76: Add a semicolon after the statement.

An explicit or implicit semicolon is expected here to end the statement.

- for (let x = 0; x < matrix.length; x++) {
+ for (let x = 0; x < matrix.length; x++); {

Likely invalid or redundant comment.

Tools
Biome

[error] 76-76: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 76-76: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/menu/SplitCell.ts (4)

13-16: LGTM!

The getValue method correctly returns an empty string as no value needs to be retrieved.


18-21: LGTM!

The isActive method correctly returns false as there is no need to check for active state.


23-35: LGTM!

The isDisabled method correctly determines if the current selection can be split based on rowspan and colspan.


51-145: LGTM!

The split method correctly handles the cell splitting operation, updating the editor's state and managing various edge cases.

packages/table-module/src/module/menu/InsertTable.ts (2)

16-16: LGTM!

The addition of the columnWidths array with a default width of 60 for each column is correct and consistent with the function's logic.


42-42: LGTM!

The addition of the columnWidths array to the returned table node is correct and consistent with the function's logic.

packages/table-module/src/module/plugin.ts (1)

226-230: LGTM!

The addition of the withSelection function to handle selection behaviors is correct and consistent with the rest of the function.

packages/table-module/src/module/render-elem/render-table.tsx (1)

58-66: LGTM!

The addition of the new properties to the table element is correct and consistent with the rest of the function's logic.

packages/basic-modules/src/modules/image/render-elem.tsx (5)

36-37: LGTM!

The change to comment out the CSS property for height is correct and consistent with the function's logic.


64-64: LGTM!

The addition of the maxWidth variable to handle image width adjustments is correct and consistent with the function's logic.


120-120: LGTM!

The change to comment out the CSS property for height is correct and consistent with the function's logic.


129-129: LGTM!

The change to comment out the CSS property for height is correct and consistent with the function's logic.


147-147: LGTM!

The change to comment out the CSS property for height is correct and consistent with the function's logic.

packages/table-module/src/constants/svg.ts (2)

45-46: LGTM! Verify SVG string length.

The MERGE_CELL_SVG constant addition looks good. Ensure that the SVG string length is optimized to avoid increasing the code size unnecessarily.


49-50: LGTM! Verify SVG string length.

The SPLIT_CELL_SVG constant addition looks good. Ensure that the SVG string length is optimized to avoid increasing the code size unnecessarily.

packages/table-module/src/module/column-resize.ts (6)

11-21: LGTM!

The getCumulativeWidths function correctly calculates the cumulative widths of columns.


26-35: LGTM!

The getColumnWidthRatios function correctly calculates the width ratios of columns.


42-60: LGTM! Verify memory management.

The observerTableResize function correctly sets up a ResizeObserver to observe table size changes. Ensure that the observer is properly disconnected to avoid memory leaks.


62-67: LGTM!

The unObserveTableResize function correctly disconnects the ResizeObserver to prevent memory leaks.


79-113: LGTM! Verify edge cases.

The onMouseDown function correctly handles the mousedown event for column resizing. Ensure that all edge cases are properly handled.


115-149: LGTM!

The onMouseMove function correctly handles the mousemove event for column resizing.

packages/core/src/editor/dom-editor.ts (1)

219-221: LGTM! Verify necessity of new condition.

The modification to the hasDOMNode function to include an additional condition checking for the attribute data-slate-string looks good. Ensure that this new condition is necessary and properly integrated.

@cycleccc
Copy link
Owner

cycleccc commented Jul 9, 2024

收到,今晚 review。

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4956621 and 451a481.

Files selected for processing (1)
  • packages/table-module/src/locale/en.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/table-module/src/locale/en.ts

Copy link
Owner

@cycleccc cycleccc left a comment

Choose a reason for hiding this comment

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

代码看了一遍,涉及table的改动挺大,只能是粗略的过了一遍,关于 render-elem 和
syncSelection 的改动应该没有影响,table 也测过了没有问题。

现在比较关键的是单测需要做适配更改。这个你有空看一下吗,其它模块的单测都是toBe的字符串匹配问题,table模块单测有两个空值引用问题。

需要的话 ,toBe 比较我可以提 pr 做适配,你可以看一下 修复一下 两个空值的问题。

@cycleccc
Copy link
Owner

需要跑一下 yarn lint --fix

@cycleccc
Copy link
Owner

我这边check到你的分支改了部分 test 文件,因为 lint 校验没过,提不上去,没法合到你的分支里,这太扯了😂

@jschyz
Copy link
Contributor Author

jschyz commented Jul 10, 2024

我这边check到你的分支改了部分 test 文件,因为 lint 校验没过,提不上去,没法合到你的分支里,这太扯了😂

确实没有 lint; table 单测完看看。

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 451a481 and 26c254b.

Files selected for processing (13)
  • packages/basic-modules/tests/image/render-elem.test.ts (2 hunks)
  • packages/core/src/editor/dom-editor.ts (1 hunks)
  • packages/table-module/tests/elem-to-html.test.ts (3 hunks)
  • packages/table-module/tests/render-elem.test.ts (1 hunks)
  • packages/table-module/src/constants/svg.ts (1 hunks)
  • packages/table-module/src/module/elem-to-html.ts (2 hunks)
  • packages/table-module/src/module/menu/DeleteCol.ts (2 hunks)
  • packages/table-module/src/module/menu/InsertCol.ts (2 hunks)
  • packages/table-module/src/module/menu/InsertTable.ts (2 hunks)
  • packages/table-module/src/module/menu/SplitCell.ts (1 hunks)
  • packages/table-module/src/module/plugin.ts (2 hunks)
  • packages/table-module/src/module/render-elem/render-cell.tsx (2 hunks)
  • packages/table-module/src/module/render-elem/render-table.tsx (4 hunks)
Files skipped from review due to trivial changes (1)
  • packages/table-module/src/module/plugin.ts
Files skipped from review as they are similar to previous changes (5)
  • packages/core/src/editor/dom-editor.ts
  • packages/table-module/src/module/elem-to-html.ts
  • packages/table-module/src/module/menu/InsertTable.ts
  • packages/table-module/src/module/menu/SplitCell.ts
  • packages/table-module/src/module/render-elem/render-cell.tsx
Additional context used
Biome
packages/table-module/src/module/menu/InsertCol.ts

[error] 68-68: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 57-57: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


[error] 68-68: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/menu/DeleteCol.ts

[error] 76-76: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 65-65: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


[error] 76-76: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/render-elem/render-table.tsx

[error] 146-146: Missing key property for this element in iterable.

The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.

(lint/correctness/useJsxKeyInIterable)


[error] 159-160: Missing key property for this element in iterable.

The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.

(lint/correctness/useJsxKeyInIterable)

Additional comments not posted (15)
packages/table-module/__tests__/render-elem.test.ts (2)

37-43: Accurate and well-documented structural change.

The test case accurately checks the rendering of the table element with the new outer div structure. The added comment provides clear context for this change.


51-53: Correct test case for full-width table rendering.

The test case correctly verifies that the table element is rendered with a width of 100% when specified.

packages/basic-modules/__tests__/image/render-elem.test.ts (2)

36-36: Correct addition of style property to image element.

The style property specifies the width of the image. The test case correctly verifies the rendering of the image element with this style.


44-44: Temporary disabling of height style property check.

The commented-out line indicates that the test case for the height style property is temporarily disabled due to an issue. This is acceptable if the issue is tracked and resolved later.

packages/table-module/__tests__/elem-to-html.test.ts (2)

Line range hint 62-92: Correct test case for converting table cell to HTML.

The test case correctly verifies the conversion of a table cell element to an HTML string with the correct attributes and content.


102-104: Correct test case for converting full-width table to HTML.

The test case correctly verifies the conversion of a full-width table element to an HTML string with the correct style attribute.

packages/table-module/src/module/menu/InsertCol.ts (4)

6-11: Necessary imports for enhanced logic.

The imports for Path from slate and filledMatrix from ../../utils are necessary for the enhanced logic for handling cell insertion and merging.


55-67: Correct logic for determining the selected cell index.

The logic correctly determines the index of the selected cell in the matrix for further operations.

Tools
Biome

[error] 57-57: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


68-89: Insert new cells and handle merged cells correctly.

The logic ensures that new cells are inserted correctly and merged cells are handled appropriately.

Tools
Biome

[error] 68-68: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 68-68: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)


108-119: Adjust column widths correctly.

The logic ensures that column widths are adjusted appropriately after inserting new cells.

packages/table-module/src/module/menu/DeleteCol.ts (2)

9-10: New imports are appropriate.

The new imports filledMatrix from ../../utils and TableCellElement, TableElement from ../custom-types are necessary for the enhanced logic of deleting columns in tables.


75-76: Add a missing semicolon.

An explicit or implicit semicolon is expected here to end the statement.

- Editor.withoutNormalizing(editor, () => {
+ Editor.withoutNormalizing(editor, () => {
+   ;

Likely invalid or redundant comment.

Tools
Biome

[error] 76-76: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 76-76: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/render-elem/render-table.tsx (2)

6-19: New imports are appropriate.

The new imports debounce from lodash.debounce, TableCursor from ../table-cursor, and several functions from ../column-resize are necessary for the enhanced logic of rendering table cells, handling cell borders, and column resizing.


68-73: Ensure the correct handling of table selection.

The variable selected is assigned using DomEditor.isNodeSelected, and the variable isSelecting is assigned using TableCursor.selection. Ensure that the logic correctly handles table selection and cursor behavior.

packages/table-module/src/constants/svg.ts (1)

45-50: New SVG icons are appropriate.

The new SVG icons MERGE_CELL_SVG and SPLIT_CELL_SVG are necessary for the new functionality of merging and splitting table cells.

@jschyz
Copy link
Contributor Author

jschyz commented Jul 10, 2024

求助,这个 test FAIL 莫名其妙

image

@cycleccc
Copy link
Owner

哈哈哈,我就是这个有点困惑才让你看下的,我今天有事应该看不了这个了,明天看看能不能解决,不能解决的话就小步快跑了。说不定看半天是jest的毛病呢,我有打算换 vtest 的,现在单测有点多,vtest 似乎更快一些。

@cycleccc
Copy link
Owner

image
看报错信息 似乎是 tableEntry 可能不存在 解构 null 报错了。
image

@jschyz
Copy link
Contributor Author

jschyz commented Jul 11, 2024

我在给 jest打断点,找到问题了,我改改

@jschyz
Copy link
Contributor Author

jschyz commented Jul 11, 2024

image

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Outside diff range, codebase verification and nitpick comments (1)
tests/setup/index.ts (1)

26-40: Ensure complete implementation of ResizeObserver mock.

The ResizeObserver mock methods are currently placeholders. If specific behavior is required for tests, consider implementing the methods or adding comments to clarify their purpose.

  observe() {
    // 可以根据需要添加具体实现
  }
  unobserve() {
    // 可以根据需要添加具体实现
  }
  disconnect() {
    // 可以根据需要添加具体实现
  }
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 26c254b and 4cd9917.

Files selected for processing (6)
  • packages/table-module/tests/menu/delete-col.test.ts (2 hunks)
  • packages/table-module/tests/menu/delete-row.test.ts (3 hunks)
  • packages/table-module/tests/menu/insert-col.test.ts (2 hunks)
  • packages/table-module/src/module/menu/DeleteCol.ts (2 hunks)
  • packages/table-module/src/module/menu/InsertCol.ts (2 hunks)
  • tests/setup/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
  • packages/table-module/tests/menu/insert-col.test.ts
Additional context used
Biome
packages/table-module/src/module/menu/InsertCol.ts

[error] 68-68: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 57-57: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


[error] 68-68: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

packages/table-module/src/module/menu/DeleteCol.ts

[error] 76-76: Expected a semicolon or an implicit semicolon after a statement, but found none

An explicit or implicit semicolon is expected here...

...Which is required to end this statement

(parse)


[error] 65-65: Unused label.

The label is not used by any break statement and continue statement.
Safe fix: Remove the unused label.

(lint/correctness/noUnusedLabels)


[error] 76-76: This code is unreachable

... because this statement will break the flow of the code beforehand

(lint/correctness/noUnreachable)

Additional comments not posted (8)
packages/table-module/src/module/menu/InsertCol.ts (1)

11-11: Import filledMatrix.

The import statement for filledMatrix has been added, which is used in the exec method.

packages/table-module/src/module/menu/DeleteCol.ts (1)

9-10: Import filledMatrix.

The import statement for filledMatrix has been added, which is used in the exec method.

packages/table-module/__tests__/menu/delete-row.test.ts (2)

9-11: Mock filledMatrix.

The filledMatrix function is mocked for testing purposes.


Line range hint 143-156:
Ensure test coverage for different scenarios.

The test case for exec method includes scenarios for removing rows. Ensure that all edge cases are covered, such as tables with merged cells or different structures.

packages/table-module/__tests__/menu/delete-col.test.ts (4)

4-4: Import statement approved.

The import of utils is correctly added.


9-11: Mocking filledMatrix function is correct.

The filledMatrix function is mocked correctly using jest.fn().


12-13: Mocked utils assignment approved.

The assignment of mockedUtils is correct.


148-151: Verify the correctness of the mocked filledMatrix implementation.

The filledMatrix mock implementation returns a nested array representing the table structure. Verify the correctness of this structure to ensure it matches the expected format.

Verification successful

Verify the correctness of the mocked filledMatrix implementation.

The mocked filledMatrix implementation in the test file accurately reflects the structure produced by the actual filledMatrix function. The nested arrays, node entries, and context information (rtl, ltr, ttb, btt) are consistent with the expected format.

  • The mocked implementation returns a nested array structure with node entries and context information that matches the real implementation of filledMatrix.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the mocked filledMatrix implementation.

# Test: Ensure the returned structure matches the expected table format.
rg --type ts 'function filledMatrix' -A 10

Length of output: 934


Script:

#!/bin/bash
# Description: Verify the correctness of the mocked filledMatrix implementation.

# Retrieve the complete implementation of the filledMatrix function.
rg --type ts 'function filledMatrix' -A 50

Length of output: 3273

@cycleccc cycleccc merged commit 1097390 into cycleccc:master Jul 11, 2024
@cycleccc
Copy link
Owner

已经发布了新版本,Cypress 的 action checkout,出问题了,我晚上回去看一下。

@jschyz
Copy link
Contributor Author

jschyz commented Jul 12, 2024

嗯嗯,什么问题

@cycleccc
Copy link
Owner

看了一下,似乎是 e2e 测试的 docker 容器版本过时了,换了最新版和node版本,现在没问题了。

@cycleccc
Copy link
Owner

@jschyz 方便加个微信不?ylf2991205548
我今天测试,发现飞书的表格在加入合并功能后复制进来渲染出了问题,这个你方便看一下嘛。
我正在搞文档,另一个飞书文档渲染 blockquote 出错的问题也得看看,这个我可以晚点看下。

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.

2 participants