feat(tables): overhaul table cells#1429
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
* 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
matthewlipski
left a comment
There was a problem hiding this comment.
Nice! Left some comments - mostly questions and some small suggestions
|
|
||
| .bn-editor [data-content-type="table"] th { | ||
| font-weight: bold; | ||
| text-align: left; |
There was a problem hiding this comment.
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.
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;
}There was a problem hiding this comment.
Makes sense, though I think it's still weird that currently changing text alignment on a header cell via the formatting toolbar has no effect between alignment left/center. I think if we want them to be centered by default, we should do it through the text alignment prop.
| blockTypeSelectItems?: BlockTypeSelectItem[] | ||
| ): JSX.Element[] => [ | ||
| <BlockTypeSelect key={"blockTypeSelect"} items={blockTypeSelectItems} />, | ||
| <TableCellMergeButton key={"tableCellMergeButton"} />, |
There was a problem hiding this comment.
I would put this at the end of the formatting toolbar instead of at the start
There was a problem hiding this comment.
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.
| > = { | ||
| type: "tableContent"; | ||
| columnWidths?: (number | undefined)[]; | ||
| headerRows?: number; |
There was a problem hiding this comment.
How come these are numbers? Isn't it only possible to have 1 of each?
There was a problem hiding this comment.
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
|
I just noticed that after setting text alignment in a cell, the selection closes and formatting toolbar disappears. I don't think it's necessary to fix before merging, but smth we may want to look into next week |
@matthewlipski I took a quick look into this to see what we can do here, and this is what is happening here: So, the updateBlock call is completely replacing the table content, which causes the selection to be lost in the replaceRange step. What I'd like out of this is 1 of two things, either make the selection more low-level so that we can set the position before & after to allow it to be in the same spot. Or, for the update block function to be smarter & not replace the whole range, just the "affected" porition, making a fine-grained update of the diff of the changes so that it knows it doesn't really need to mess with the selection at all. Maybe even both. Definitely think it is out of scope for the moment |
There was a problem hiding this comment.
had a quick glance at final state. two questions:
- example links to https://www.blocknotejs.org/docs/editor-basics/tables which doesn't exist
- did you want to simplify the types (we discussed this last week) or keep as is for now?
Also; enjoy the fun of closing all related issues 🙌🎉
Whoops, meant to link to advanced tables, I renamed it somewhere
Sorry, meant to write something about why I decided not to do this atm. Basically it came down to:
|
💖 This feature is sponsored by Agree.com
This PR is attempting to address the following for table cells:
Screen.Recording.2025-02-20.at.14.19.23.mp4