[WIKI-491] [WIKI-496] [WIKI-499] refactor: tables width and selection UI #7274
Conversation
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
WalkthroughThis update refactors table-related logic and styling in the editor. It introduces a default column width constant, adjusts table and cell selection handling, restructures CSS for tables and drag-drop, and adds a ProseMirror plugin for outlining selected table cells. Several selectors and command signatures are updated to streamline table management and visual consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant TableCellSelectionOutlinePlugin
participant TableView
User->>Editor: Selects table cells
Editor->>TableCellSelectionOutlinePlugin: Updates selection state
TableCellSelectionOutlinePlugin->>Editor: Applies border classes to selected cells
Editor->>TableView: Renders table with outlined cell selections
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 comments (1)
packages/editor/src/core/extensions/table/table/utilities/create-table.ts (1)
22-42: Every row currently re-uses the same cell nodes
headerCellsandcellsare created once and inserted into all
rows. ProseMirror expects each node instance to appear only once in
the document tree; re-using them can lead to
“Node may not be reused” errors and unexpected mutations when one cell
is edited.A minimal fix is to copy the nodes per row:
- for (let index = 0; index < rowsCount; index += 1) { - rows.push(types.row.createChecked(null, withHeaderRow && index === 0 ? headerCells : cells)); - } + for (let rowIndex = 0; rowIndex < rowsCount; rowIndex += 1) { + const source = withHeaderRow && rowIndex === 0 ? headerCells : cells; + + // copy() produces a structurally identical node with a new identity + const rowCells = source.map((cell) => cell.copy()); + + rows.push(types.row.createChecked(null, rowCells)); + }
🧹 Nitpick comments (5)
packages/editor/src/styles/variables.css (1)
189-193: Consider consistent padding for full-width blocks.The full-width block styling uses different padding values for start and end (
padding-inline-enduses--wide-content-marginwhich is 96px). This might create asymmetric layouts. Consider using the same calculation for both sides:& > .editor-full-width-block { max-width: 100%; padding-inline-start: calc((100% - var(--editor-content-width)) / 2); - padding-inline-end: var(--wide-content-margin); + padding-inline-end: calc((100% - var(--editor-content-width)) / 2); }packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts (2)
5-5: Remove React dependency for better modularity.The import of
CSSPropertiesfrom React seems unnecessary since this is a ProseMirror plugin that doesn't use React. Consider defining the styles inline or creating a local type:-import type { CSSProperties } from "react"; +type CSSProperties = Record<string, string | number>;This improves modularity and removes an unnecessary dependency.
Also applies to: 35-80
118-138: Consider performance optimization for DOM element creation.Creating DOM elements on every selection change could impact performance with large tables or frequent selections. Consider:
- Reusing DOM elements where possible
- Using CSS classes instead of inline styles
- Implementing a caching mechanism for border elements
This would reduce garbage collection pressure and improve performance.
packages/editor/src/core/extensions/table/table/utilities/create-table.ts (1)
25-27: Unnecessary null-checks
createCellalways returns aProsemirrorNode, so the surrounding
if (cell) { … }blocks can be removed to simplify the loop.packages/editor/src/styles/table.css (1)
43-47: Visual contrast check for selection background
rgba(var(--color-primary-100), 0.2)applies a 20 % alpha overlay to
the whole table. On light themes this can be very subtle; on dark
themes it may be too strong. Consider usingcolor-mix()with the
surface background or adjusting the alpha via design tokens so that
contrast meets WCAG AA across themes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/editor/src/core/extensions/side-menu.ts(1 hunks)packages/editor/src/core/extensions/table/table-cell.ts(2 hunks)packages/editor/src/core/extensions/table/table/index.ts(1 hunks)packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts(1 hunks)packages/editor/src/core/extensions/table/table/table-view.tsx(1 hunks)packages/editor/src/core/extensions/table/table/table.ts(3 hunks)packages/editor/src/core/extensions/table/table/utilities/create-table.ts(2 hunks)packages/editor/src/core/helpers/editor-commands.ts(1 hunks)packages/editor/src/core/plugins/drag-handle.ts(3 hunks)packages/editor/src/styles/drag-drop.css(2 hunks)packages/editor/src/styles/table.css(2 hunks)packages/editor/src/styles/variables.css(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
packages/editor/src/core/extensions/table/table/table-view.tsx (1)
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
packages/editor/src/styles/drag-drop.css (2)
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
Learnt from: vineetk13
PR: makeplane/plane#6391
File: web/styles/react-day-picker.css:249-282
Timestamp: 2025-01-17T05:17:51.953Z
Learning: In the date range picker's CSS, left/right positioning for cell background effects (like in `.rdp-range_start::before`, `.rdp-range_middle::before`, `.rdp-range_end::before`) should use physical properties instead of logical properties, as these create visual effects that should remain consistent regardless of text direction.
packages/editor/src/styles/variables.css (1)
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
packages/editor/src/styles/table.css (2)
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
Learnt from: vineetk13
PR: makeplane/plane#6391
File: web/styles/react-day-picker.css:249-282
Timestamp: 2025-01-17T05:17:51.953Z
Learning: In the date range picker's CSS, left/right positioning for cell background effects (like in `.rdp-range_start::before`, `.rdp-range_middle::before`, `.rdp-range_end::before`) should use physical properties instead of logical properties, as these create visual effects that should remain consistent regardless of text direction.
🧬 Code Graph Analysis (2)
packages/editor/src/core/extensions/table/table-cell.ts (1)
packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts (1)
TableCellSelectionOutlinePlugin(88-150)
packages/editor/src/core/extensions/table/table/table.ts (2)
packages/editor/src/core/extensions/table/table/utilities/create-table.ts (1)
createTable(15-45)packages/editor/src/core/extensions/table/table/index.ts (1)
DEFAULT_COLUMN_WIDTH(3-3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
packages/editor/src/core/extensions/table/table/index.ts (1)
3-3: LGTM! Good centralization of default values.The
DEFAULT_COLUMN_WIDTHconstant provides a single source of truth for table column widths, making the codebase more maintainable and consistent.packages/editor/src/core/extensions/side-menu.ts (1)
134-134: Improved DOM targeting precision.The change from
.table-wrappertotableprovides more precise element targeting, which aligns with the broader table refactoring effort. This approach is more semantically correct and reduces dependency on CSS class names.packages/editor/src/core/extensions/table/table/table-view.tsx (1)
390-390: Good styling improvements for table layout.The addition of
editor-full-width-blockclass supports the full-width table layout design, and the scrollbar size change fromscrollbar-mdtoscrollbar-smprovides a more refined visual appearance.packages/editor/src/core/helpers/editor-commands.ts (1)
112-113: Good refactoring to remove hardcoded values.Removing the explicit
columnWidth: 150parameter improves maintainability by allowing the table extension to handle default column width internally via theDEFAULT_COLUMN_WIDTHconstant. This decouples the command from implementation details.packages/editor/src/core/extensions/table/table-cell.ts (2)
4-5: Good import organization for the new plugin.The import is properly placed in the local imports section with clear commenting.
52-54: Excellent UX enhancement with proper plugin integration.The
TableCellSelectionOutlinePluginintegration provides visual feedback for table cell selections, improving the editing experience. The plugin is properly registered through theaddProseMirrorPluginsmethod and correctly passes the editor instance.packages/editor/src/styles/drag-drop.css (1)
38-71: CSS changes look good and align with table UI improvements.The exclusion of
.table-wrapperfrom the general selected node styles and its specific inclusion with image-related nodes for offset and background styling is consistent with the PR objectives to improve table selection UI.packages/editor/src/core/extensions/table/table/table.ts (1)
32-32: Good refactoring to simplify the table insertion API.The removal of the
columnWidthparameter from the command signature and the use ofDEFAULT_COLUMN_WIDTHconstant promotes consistency across the codebase. The switch to an options object forcreateTableimproves maintainability.Also applies to: 46-46, 115-123
packages/editor/src/core/plugins/drag-handle.ts (1)
19-19: Improved semantic targeting by usingtableelement selector.The consistent change from
.table-wrappertotableacross all three locations improves reliability by targeting the actual table element rather than relying on a wrapper class. This is a more robust approach for table detection.Also applies to: 93-93, 102-102
packages/editor/src/styles/table.css (1)
1-41: Confirm that the build pipeline supports native / PostCSS nestingThe file now relies heavily on nested selectors (
table { … },
&.ProseMirror-selectednode { … }, etc.) which plain CSS does not
yet support in all target browsers.
If the repository isn’t already usingpostcss-nestingor the new CSS
Nesting spec behind a compiler step, this change will break the style
sheet at runtime.Please double-check the tooling or move the rules to a pre-processed
.scss/.pcssfile.
packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts
Outdated
Show resolved
Hide resolved
packages/editor/src/core/extensions/table/table/utilities/create-table.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts (1)
38-38: Good fix: Plugin key is now appropriately specific.The plugin key has been updated from the generic "selection" to "table-cell-selection-outline", addressing the previous review feedback.
🧹 Nitpick comments (2)
packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts (2)
18-19: Improve variable naming for better readability.The variable names
testRightandtestBottomdon't clearly convey their purpose as cell span offsets.- const testRight = cellW; - const testBottom = width * cellH; + const rightOffset = cellW; + const bottomOffset = width * cellH;And update their usage accordingly in lines 22 and 24.
21-24: Consider adding comments for complex cell adjacency logic.The cell adjacency calculations handle spanning cells correctly but are complex to understand without context.
+ // Calculate adjacent cell positions accounting for cell spans const topCell = cellIndex >= width ? tableMap.map[cellIndex - width] : undefined; - const bottomCell = cellIndex < width * height - testBottom ? tableMap.map[cellIndex + testBottom] : undefined; + const bottomCell = cellIndex < width * height - bottomOffset ? tableMap.map[cellIndex + bottomOffset] : undefined; const leftCell = cellIndex % width > 0 ? tableMap.map[cellIndex - 1] : undefined; - const rightCell = cellIndex % width < width - testRight ? tableMap.map[cellIndex + testRight] : undefined; + const rightCell = cellIndex % width < width - rightOffset ? tableMap.map[cellIndex + rightOffset] : undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts(1 hunks)packages/editor/src/styles/table.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/styles/table.css
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/editor/src/core/extensions/table/table/plugins/selection-outline.plugin.ts (5)
1-5: LGTM: Clean imports and dependencies.The imports are well-organized and include all necessary ProseMirror and TipTap dependencies for the plugin functionality.
45-51: Efficient early returns prevent unnecessary processing.The plugin correctly handles non-editable states, missing tables, and unchanged states to avoid unnecessary decoration recalculation.
76-76: Decoration implementation follows ProseMirror best practices.The node decoration with CSS classes is properly implemented and will integrate well with the corresponding CSS styles.
84-88: Plugin structure follows ProseMirror conventions correctly.The plugin properly exposes decorations through the props.decorations method and maintains state through the plugin key.
60-63: Consider performance impact for large tables.The plugin recalculates all decorations on every selection change within a table. For very large tables with many selected cells, this could impact performance.
#!/bin/bash # Description: Check if there are any performance optimizations or throttling mechanisms in similar table plugins # Search for performance-related patterns in table extensions rg -A 5 -B 5 "performance|throttle|debounce|optimize" --type ts packages/editor/src/core/extensions/table/
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/editor/src/core/extensions/table/table/plugins/table-selection-outline/plugin.ts (1)
41-46: Consider performance optimization for large table selections.The current implementation creates decorations for every selected cell individually. For large table selections, consider batching decoration creation or using range-based decorations to improve performance.
- // Then, add decorations with appropriate border classes - selection.forEachCell((node, pos) => { - const start = pos - table.pos - 1; - const classes = getCellBorderClasses(start, selectedCells, tableMap); - - decorations.push(Decoration.node(pos, pos + node.nodeSize, { class: classes.join(" ") })); - }); + // Then, add decorations with appropriate border classes + const cellDecorations = []; + selection.forEachCell((node, pos) => { + const start = pos - table.pos - 1; + const classes = getCellBorderClasses(start, selectedCells, tableMap); + + if (classes.length > 0) { + cellDecorations.push(Decoration.node(pos, pos + node.nodeSize, { class: classes.join(" ") })); + } + }); + decorations.push(...cellDecorations);packages/editor/src/core/extensions/table/table/plugins/table-selection-outline/utils.ts (2)
20-23: Add error handling for edge case.The
indexOfoperation could return -1 for malformed table structures. Consider adding a more descriptive error or logging to help debug issues when cell positions are not found in the table map.// Find the index of our cell in the flat tableMap.map array // tableMap.map contains start positions of all cells in row-by-row order const cellIndex = tableMap.map.indexOf(cellStart); // Safety check: if cell position not found in table map, return empty object - if (cellIndex === -1) return {}; + if (cellIndex === -1) { + console.warn(`Cell position ${cellStart} not found in table map`); + return {}; + }
9-11: Export the utility function for potential reuse.The
getAdjacentCellPositionsfunction could be useful in other table-related features. Consider exporting it for broader use within the table extension.- const getAdjacentCellPositions = ( + export const getAdjacentCellPositions = ( cellStart: number, tableMap: TableMap ): { top?: number; bottom?: number; left?: number; right?: number } => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/editor/src/core/extensions/table/table-cell.ts(2 hunks)packages/editor/src/core/extensions/table/table/plugins/table-selection-outline/plugin.ts(1 hunks)packages/editor/src/core/extensions/table/table/plugins/table-selection-outline/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/table/table-cell.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/editor/src/core/extensions/table/table/plugins/table-selection-outline/plugin.ts (1)
packages/editor/src/core/extensions/table/table/plugins/table-selection-outline/utils.ts (1)
getCellBorderClasses(50-75)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/editor/src/core/extensions/table/table/plugins/table-selection-outline/plugin.ts (2)
19-25: Good optimization with early returns and state preservation.The plugin correctly handles non-editable states and unchanged documents, with proper state preservation when no table is found.
35-38: Verify position calculation consistency.The position calculation
pos - table.pos - 1converts absolute document positions to table-relative positions. Ensure this calculation is consistent with howTableMapexpects positions.#!/bin/bash # Description: Verify how ProseMirror TableMap handles position calculations # Expected: Confirm that table-relative positions are calculated correctly rg -A 10 -B 5 "table\.pos" --type ts rg -A 10 -B 5 "TableMap\.get" --type tspackages/editor/src/core/extensions/table/table/plugins/table-selection-outline/utils.ts (3)
28-29: Mathematical calculations are correct.The coordinate conversion from flat array index to 2D grid coordinates using integer division and modulo operations is mathematically sound and well-documented.
31-47: Boundary checks are comprehensive and correct.The boundary checks for each direction (top, bottom, left, right) properly handle table edges by checking row and column bounds before calculating adjacent positions.
50-75: Border class logic is sound and efficient.The function correctly determines which borders to show by checking if adjacent cells are selected or don't exist. The logic ensures that selection outlines only appear at the boundary of the selection.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/editor/src/core/extensions/table/plugins/table-selection-outline/utils.ts (1)
50-75: Consider optimizing array membership checks for better performance.The function logic is correct for determining border classes, but the repeated
selectedCells.includes()calls could be expensive for large selections sinceincludes()has O(n) complexity.Consider converting the
selectedCellsarray to aSetfor O(1) membership checks:-export const getCellBorderClasses = (cellStart: number, selectedCells: number[], tableMap: TableMap): string[] => { +export const getCellBorderClasses = (cellStart: number, selectedCells: number[] | Set<number>, tableMap: TableMap): string[] => { const adjacent = getAdjacentCellPositions(cellStart, tableMap); const classes: string[] = []; + const selectedSet = selectedCells instanceof Set ? selectedCells : new Set(selectedCells); // Add border-right if right cell is not selected or doesn't exist - if (adjacent.right === undefined || !selectedCells.includes(adjacent.right)) { + if (adjacent.right === undefined || !selectedSet.has(adjacent.right)) { classes.push("selectedCell-border-right"); } // Apply similar changes to other border checks...packages/editor/src/core/extensions/table/plugins/table-selection-outline/plugin.ts (2)
14-58: Well-structured plugin implementation with room for performance optimization.The plugin correctly implements the ProseMirror plugin pattern and handles table cell selection outlining. The logic is sound and follows best practices for state management and decoration application.
Consider these optimizations for better performance:
- Optimize selectedCells collection: Convert to Set for faster lookups:
- const selectedCells: number[] = []; + const selectedCells = new Set<number>(); // First, collect all selected cell positions selection.forEachCell((_node, pos) => { const start = pos - table.pos - 1; - selectedCells.push(start); + selectedCells.add(start); });
- Add early return for single cell selections: Single cells don't need border outlining:
// First, collect all selected cell positions selection.forEachCell((_node, pos) => { const start = pos - table.pos - 1; selectedCells.add(start); }); + // Skip decoration for single cell selections + if (selectedCells.size === 1) return {};
- Consider caching decorations: If the same selection persists across multiple state changes, you could cache the decoration set to avoid recalculation.
20-25: Consider clarifying the conditional logic for better readability.The nested conditional logic works correctly but could be more explicit about the different scenarios.
Consider restructuring for better clarity:
- if (!table || !hasDocChanged) { - return table === undefined ? {} : prev; - } + // No table context - clear decorations + if (!table) return {}; + + // No changes - keep previous decorations + if (!hasDocChanged) return prev;This makes the two distinct scenarios more explicit: clearing decorations vs. preserving them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/editor/src/core/extensions/table/plugins/table-selection-outline/plugin.ts(1 hunks)packages/editor/src/core/extensions/table/plugins/table-selection-outline/utils.ts(1 hunks)packages/editor/src/core/extensions/table/table-cell.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/extensions/table/table-cell.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/editor/src/core/extensions/table/plugins/table-selection-outline/plugin.ts (1)
packages/editor/src/core/extensions/table/plugins/table-selection-outline/utils.ts (1)
getCellBorderClasses(50-75)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/editor/src/core/extensions/table/plugins/table-selection-outline/utils.ts (1)
9-48: LGTM! Well-implemented adjacency calculation with proper edge handling.The
getAdjacentCellPositionsfunction correctly:
- Converts flat table map indices to 2D coordinates using division and modulo
- Handles table edges by returning
undefinedfor non-existent adjacent cells- Includes comprehensive inline documentation explaining the coordinate system
The bounds checking logic is sound and prevents accessing cells outside the table boundaries.
aaryan610
changed the title
Description
Type of Change
Media
Screen.Recording.2025-06-26.at.18.07.20.mov
Screen.Recording.2025-06-26.at.18.04.49.mov
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style