-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
feat(tables): overhaul table cells #1429
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/core/src/api/blockManipulation/commands/updateBlock/updateBlock.test.ts
Show resolved
Hide resolved
* fix: table cell merging * fix: better merging of styled text in table cells
* fix: table cell merging * fix: better merging of styled text in table cells
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Left some comments - mostly questions and some small suggestions
@@ -31,6 +31,19 @@ type ToolbarButtonType = { | |||
| { children: ReactNode; label?: string } | |||
| { children?: undefined; label: string } | |||
); | |||
|
|||
type ButtonType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rename to MenuButtonType
Also we should revisit the components context at some point and try to make things more generic - we probably don't need a bunch of different buttons in different places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this button was my attempt at a generic button, but couldn't find the right level to place it at
@@ -160,10 +159,10 @@ export const Table = createBlockSpecFromStronglyTypedTiptapNode( | |||
TableExtension, | |||
TableParagraph, | |||
TableHeader.extend({ | |||
content: "tableContent", | |||
content: "tableContent*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this change? Since table cells should only be able to have a single paragraph in them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment explaining why. But, essentially it is because when merging cells, prosemirror-tables does a naive concat of the content of the table cells. We manually fix this up when doing nodeToBlock though to get it back to a single item again.
@@ -156,7 +156,6 @@ Tippy popups that are appended to document.body directly | |||
|
|||
.bn-editor [data-content-type="table"] th { | |||
font-weight: bold; | |||
text-align: left; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this still needs to be set as table headers default to center alignment, so they're still centered even when the text alignment is set to left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is a style thing, but table headers are usually centered.
Also, I didn't want this CSS to conflict with manually setting the text alignment on the cell level given it is a low specificity selector:
[data-text-alignment="right"] {
justify-content: flex-end;
text-align: right;
}
import { TextAlignButton } from "./DefaultButtons/TextAlignButton.js"; | ||
import { FormattingToolbarProps } from "./FormattingToolbarProps.js"; | ||
|
||
export const getFormattingToolbarItems = ( | ||
blockTypeSelectItems?: BlockTypeSelectItem[] | ||
): JSX.Element[] => [ | ||
<BlockTypeSelect key={"blockTypeSelect"} items={blockTypeSelectItems} />, | ||
<TableCellMergeButton key={"tableCellMergeButton"} />, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this at the end of the formatting toolbar instead of at the start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not sure here. I want to make the feature more known since it only ever shows for table cells. I feel like that follows the existing pattern like the file specific buttons.
export type PartialTableContent< | ||
I extends InlineContentSchema, | ||
S extends StyleSchema = StyleSchema | ||
> = { | ||
type: "tableContent"; | ||
columnWidths?: (number | undefined)[]; | ||
headerRows?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come these are numbers? Isn't it only possible to have 1 of each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this here: #1425
But, the idea is that we could in the future support multiple headerRows and headerColumns, but for now we only expose in the UI a single toggle for this. Just gives us more room in the future to implement something more complex if needed
import { useBlockNoteEditor } from "../../../../hooks/useBlockNoteEditor.js"; | ||
import { DragHandleMenuProps } from "../DragHandleMenuProps.js"; | ||
|
||
export const TableRowHeaderItem = < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 things:
- Imo would be nice to also have this on the 1st row table handle (same for column headers)
- Not specific to this component, but menu items in general look wonky with ShadCN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Good idea
- What specifically is wonky? I'm not familiar with it enough to know what feels off about it.
This PR is attempting to address the following for table cells:
Screen.Recording.2025-02-20.at.14.19.23.mp4