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 copy table related issues #169

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

GW00234847
Copy link

@GW00234847 GW00234847 commented Feb 20, 2024

Description
When we tried to use this library on our project, we encountered some issues, so I temporarily fixed them through some scripts, here comes the issues.
The following issues have been fixed: Page collapse issues caused by copied table:

  1. col span & row span greater than 1, if click enter, page will collapse
  2. TH element will be serialized like tr
  3. The last empty cell missing issue
  4. Link inside table cell can not correctly respond to enter event issue
  5. Drag column will cause cell height update, but left border with drag function will stay the same
  6. Some copied table with colgroup can not show column header because it just set all col width to 0.

Issue

Fixes: (link to issue)

Example

  1. & 2.
    before
    copy_table_rowspan_colspan_issue

after
copy_table_rowspan_colspan_issue_after

before
last_missing_cell

after
last_missing_cell_after

  1. Link issue
    before
    Link_enter

after
Link_enter_after

  1. Dragging column will cause cell height updating, but the left border with drag function will stay the same

before
drag_table_column

after
drag_table_column_after

  1. Copied table with colgroup can not drag column
    before
    partial_table_col

after
partial_table_col_after

Context
1: The raw HTML for the table, it will not display an empty dom cell for colspan or rowspan that is greater than 1, but just reduce the corresponding number of the cell. But in editablejs, the table will render every one of the cells even if it may ignored by the raw table. My solution is to fill the missing cell, calculate each line's colspan and rowspan, and if it is greater than 1, add another td empty element with displaynone||||||, so when displaying the cell, it will also need to check if it contains displaynone|||||| to hide that cell. Once the data structure works fit to editablejs, it works fine.
2: TH works as a cell, so just need to remove- it from TR to TD. So TH will work as expected.
3: Calculate the total cells of the first row of the table, when it comes to the final line of the table, check if the cell number fits = the same as the first row, if not, patch the missing cell.
4: when copying content into editablejs, its data structure didn't match what the editablejs requires, for it missing another wrapper outside children, it should be wrapped by children with children as the first element.
5: When dragging columns, it didn't update the height of the left border cell, however height of cells might update, whether gets smaller or higher. So it should recalculate the left border's height.
6: If the table has been wrapped inside by colgroup, it will auto-set each cols' with to 0 which finally causes column's width to 0, so what should be done is to check if there are any colgroup inside each table, and to reset each column's width to offsetWidth
Checks

  • The new code matches the existing patterns and styles.
  • The tests are exactly the same result as the original library with pnpm test.
  • The linter passes with pnpm lint. (Fix errors with pnpm fix.)
  • The relevant examples still work. (Run examples with pnpm start.)
  • You've added a changeset if changing functionality. (Add one with pnpm changeset.)

Copy link

changeset-bot bot commented Feb 20, 2024

🦋 Changeset detected

Latest commit: 815d607

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 23 packages
Name Type
@editablejs/deserializer Patch
@editablejs/plugin-table Patch
@editablejs/docs Patch
@editablejs/editor Patch
@editablejs/plugin-alignment Patch
@editablejs/plugin-blockquote Patch
@editablejs/plugin-codeblock Patch
@editablejs/plugin-context-menu Patch
@editablejs/plugin-font Patch
@editablejs/plugin-heading Patch
@editablejs/plugin-hr Patch
@editablejs/plugin-image Patch
@editablejs/plugin-indent Patch
@editablejs/plugin-leading Patch
@editablejs/plugin-link Patch
@editablejs/plugin-list Patch
@editablejs/plugin-mark Patch
@editablejs/plugin-mention Patch
@editablejs/plugins Patch
@editablejs/plugin-title Patch
@editablejs/plugin-history Patch
@editablejs/plugin-toolbar Patch
@editablejs/plugin-yjs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@GW00234847 GW00234847 marked this pull request as draft February 20, 2024 09:11
@GW00234847 GW00234847 marked this pull request as ready for review February 20, 2024 09:12
@GW00234847 GW00234847 marked this pull request as draft February 20, 2024 09:13
@GW00234847 GW00234847 marked this pull request as ready for review February 20, 2024 09:16
@GW00234847 GW00234847 marked this pull request as draft February 20, 2024 09:23
@big-camel big-camel marked this pull request as ready for review February 20, 2024 09:33
const children = node.children;
for (let i = 0; i < children.length; i++) {
const child = children[i];
if (child.nodeName === 'TABLE') {
Copy link
Member

Choose a reason for hiding this comment

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

那如果table里面还有嵌套的table呢?这里是只考虑了一层嵌套关系吗?

Copy link
Author

Choose a reason for hiding this comment

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

对,满足现有需求够用了

packages/deserializer/src/html.ts Outdated Show resolved Hide resolved
newCell.setAttribute('colspan', '1')
newCell.setAttribute('rowspan', '1')
// 设置当前newCell的文本为displaynone
newCell.textContent = 'displaynone||||||' + rowIndex + '||||||' + cellIndex
Copy link
Member

Choose a reason for hiding this comment

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

这里太hack了,其实我没太看懂这里把合并行和列拉平的用意

Copy link
Author

Choose a reason for hiding this comment

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

这里需要补充原始html缺失的单元格来满足当前的数据结构,这个缺失的单元格需要加一个标识让后续展示的时候有一个可识别的内容来告诉editablejs这部分需要隐藏展示,也就是对应后面的这段隐藏逻辑,
image

Copy link
Author

Choose a reason for hiding this comment

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

或者你那有好的思路,也可以参考下

Copy link
Member

Choose a reason for hiding this comment

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

或者你那有好的思路,也可以参考下

我提交了一版,整体包含了我的思路,我这儿没有测试数据。你可以先帮忙测试看看。

Copy link
Author

Choose a reason for hiding this comment

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

或者你那有好的思路,也可以参考下

我提交了一版,整体包含了我的思路,我这儿没有测试数据。你可以先帮忙测试看看。
我测试了基础的表格复制粘贴也不行,没有rowspan或者colspan的无法复制,

或者你那有好的思路,也可以参考下

我提交了一版,整体包含了我的思路,我这儿没有测试数据。你可以先帮忙测试看看。

我这运行报错了,复制部分表格的时候slate报错了,复制全部表格的时候,表格样式丢失了

部分表格:
image

全部表格:
image
image

测试数据参考截图,也可以多增加一些colspan 和rowspan,在里面输入回车看是否可行

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.

None yet

2 participants