fix(table): fill in the hidden tablecell #343
Conversation
|
WalkthroughThe changes in this pull request focus on enhancing the handling of table functionality within the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
.changeset/famous-dolls-shave.md (1)
6-6: Consider enhancing the commit message descriptionWhile the message indicates the fix, it would be more helpful to include:
- What was the specific issue with hidden table cells
- How it was addressed (e.g., "Added support for creating hidden cells when colSpan > 1")
- Impact on users/behavior
Consider expanding it to:
-fix(table): fill in the hidden table cell +fix(table): properly handle hidden cells for tables with colspan + +When a table cell has colspan > 1, ensure hidden cells are properly +created and filled, maintaining correct table structure. This fixes +issues with table rendering and manipulation when using colspan.packages/table-module/src/module/parse-elem-html.ts (2)
66-67: Add validation for colSpan valueThe colSpan value should be validated to prevent potential issues with invalid values.
Add validation:
// @ts-ignore - const colSpan = child.colSpan || 1 + const colSpan = Math.max(1, Math.floor(child.colSpan || 1))
139-144: Consider extracting the default width as a named constantThe default width of 180 is hardcoded. Consider extracting it as a named constant for better maintainability and documentation.
Add at the top of the file:
+ const DEFAULT_CELL_WIDTH = 180; // Width in pixels for table cellsThen update the width calculation:
- const width = parseInt(getStyleValue($(td), 'width') || '180', 10) + const width = parseInt(getStyleValue($(td), 'width') || String(DEFAULT_CELL_WIDTH), 10)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.changeset/famous-dolls-shave.md(1 hunks)packages/table-module/src/module/parse-elem-html.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/table-module/src/module/parse-elem-html.ts
[error] 85-86: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (2)
.changeset/famous-dolls-shave.md (1)
1-4: LGTM on version bumps
The patch version bumps are appropriate since this is a bug fix that maintains backward compatibility.
packages/table-module/src/module/parse-elem-html.ts (1)
96-96: LGTM: Proper assignment of processed table cells
The change ensures that only properly processed table cells, including hidden ones, are assigned to the row.
| if ( | ||
| nextChild | ||
| && DomEditor.getNodeType(nextChild) === 'table-cell' | ||
| // @ts-ignore | ||
| && nextChild.style?.display === 'none' | ||
| ) { | ||
| // 已有隐藏的 table-cell，无需补充 | ||
| continue | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and simplify condition block
The type assertion and condition block can be improved:
- Replace unsafe type assertion with proper type guard
- Remove unnecessary continue statement
Apply this diff:
if (
nextChild
&& DomEditor.getNodeType(nextChild) === 'table-cell'
- // @ts-ignore
- && nextChild.style?.display === 'none'
+ && 'hidden' in nextChild
+ && nextChild.hidden === true
) {
- // 已有隐藏的 table-cell，无需补充
- continue
- } else {
+ // Skip if hidden cell exists
+ } else {Committable suggestion skipped: line range outside the PR's diff.
| const tableCellChildren: TableCellElement[] = [] | ||
|
|
||
| for (let i = 0; i < children.length; i += 1) { | ||
| const child = children[i] | ||
|
|
||
| // 确保是 table-cell 类型 | ||
| if (DomEditor.getNodeType(child) === 'table-cell') { | ||
| // @ts-ignore | ||
| const colSpan = child.colSpan || 1 | ||
|
|
||
| tableCellChildren.push(child as TableCellElement) // 先添加当前单元格 | ||
|
|
||
| // 如果 colSpan > 1，检查是否存在足够的隐藏 table-cell | ||
| for (let j = 1; j < colSpan; j += 1) { | ||
| const nextChild = children[i + j] | ||
|
|
||
| if ( | ||
| nextChild | ||
| && DomEditor.getNodeType(nextChild) === 'table-cell' | ||
| // @ts-ignore | ||
| && nextChild.style?.display === 'none' | ||
| ) { | ||
| // 已有隐藏的 table-cell，无需补充 | ||
| continue | ||
| } else { | ||
| // 补齐缺少的隐藏 table-cell | ||
| tableCellChildren.push({ | ||
| type: 'table-cell', | ||
| children: [{ text: '' }], | ||
| hidden: true, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add comprehensive test cases for colSpan scenarios
The current test suite does not cover the following critical scenarios:
- Table with multiple cells having colSpan > 1
- Existing hidden cells
- Missing hidden cells
- Edge cases: colSpan = 0, negative values, non-numeric values
🔗 Analysis chain
Verify table structure with various colSpan configurations
The changes look good but should be thoroughly tested with different scenarios.
Please verify these test cases:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing table-related test files
echo "Searching for table-related test files..."
fd -e test.ts -e spec.ts . | grep -i table
# Search for table-related test cases
echo "Searching for colSpan-related test cases..."
rg -l "colSpan.*table" -t ts
# Ensure test coverage for various scenarios:
echo "Test scenarios to verify:"
cat << EOF
1. Table with single colSpan > 1
2. Table with multiple cells having colSpan > 1
3. Table with existing hidden cells
4. Table with missing hidden cells
5. Edge cases: colSpan = 0, negative values, non-numeric values
EOF
Length of output: 1257
Script:
#!/bin/bash
# List of table-related test files identified previously
TEST_FILES=(
packages/table-module/__tests__/elem-to-html.test.ts
packages/table-module/__tests__/menu/delete-col.test.ts
packages/table-module/__tests__/menu/delete-row.test.ts
packages/table-module/__tests__/menu/delete-table.test.ts
packages/table-module/__tests__/menu/full-width.test.ts
packages/table-module/__tests__/menu/insert-col.test.ts
packages/table-module/__tests__/menu/insert-row.test.ts
packages/table-module/__tests__/menu/insert-table.test.ts
packages/table-module/__tests__/menu/table-header.test.ts
packages/table-module/__tests__/parse-html.test.ts
packages/table-module/__tests__/plugin.test.ts
packages/table-module/__tests__/render-elem.test.ts
)
# Define test scenarios
SCENARIOS=(
"single colSpan > 1"
"multiple cells having colSpan > 1"
"existing hidden cells"
"missing hidden cells"
"colSpan = 0"
"colSpan < 0"
"non-numeric colSpan"
)
# Search each test file for each scenario
for file in "${TEST_FILES[@]}"; do
echo "Checking $file for test scenarios..."
for scenario in "${SCENARIOS[@]}"; do
rg -i "$scenario" "$file" && echo "✅ $scenario found in $file" || echo "❌ $scenario NOT found in $file"
done
done
Length of output: 29378
🧰 Tools
🪛 Biome
[error] 85-86: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
* fix(table): fill in the hidden tablecell * Create famous-dolls-shave.md
Changes Overview
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
Summary by CodeRabbit
Bug Fixes
Improvements