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

feat: Table row/column extension buttons #1172

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

Conversation

matthewlipski
Copy link
Collaborator

@matthewlipski matthewlipski commented Oct 21, 2024

Description

This PR improves a number of things related to tables. Mainly, "extend buttons" have been added 🙌:

tables

Other features:

Prosemirror tables

As part of this PR, we also contributed an improvement to Prosemirror-tables: ProseMirror/prosemirror-tables#253

remaining todos

  • Update e2e test screenshots
  • test on safari
  • resize cursor for shadcn
  • test shadcn / ariakit

closes #830
closes #856

Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Oct 25, 2024 8:07pm
blocknote-website ✅ Ready (Inspect) Visit Preview Oct 25, 2024 8:07pm

Copy link
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

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

Looks great already!! Nice.

Replies to your questions and general feedback:

Maybe change so that extend buttons only appear when hovering last row/column

Yes, I think that would be great, nice challenge to see if we can do that without making it "messy"

Naming

Yes, good idea to revisit

Misc:

  • UX is clunky when width is larger than document width. I think we should make the table scrollable? Otherwise the columnwidths don't make any sense when the document width is "too small". This should also solve that resizing a column is now a weird experience when the table is too wide
  • I think we need to drag to far for the add / remove to "trigger". Maybe try sth like 1/3 of width / height of the cell that will be added
  • add colwidth to unit tests? (i.e.: htmlconversions and nodeconversions)
  • (nice to have): drag to remove columns / rows as long as they're empty

packages/core/src/api/nodeConversions/nodeToBlock.ts Outdated Show resolved Hide resolved
packages/core/src/api/nodeConversions/blockToNode.ts Outdated Show resolved Hide resolved
packages/core/src/schema/blocks/types.ts Outdated Show resolved Hide resolved
document.body.removeEventListener("mouseup", callback);
};
}, [
editingState?.clickOnly,
Copy link
Collaborator

Choose a reason for hiding this comment

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

a) it looks like we set something on the state to then trigger a useEffect, that's not a very clear pattern I think
b) can't we just use the onClick handler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we have a choice here because we need the mousemove and mouseup handlers to be attached to the document body, because the mouse cursor is not always above the extend button while dragging. This is also why we can't use a click event, as those only fire when the same element is hovered on both the mouse down and mouse up.

function useExtendButtonPosition(
orientation: "row" | "col",
show: boolean,
referencePosCell: DOMRect | null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it's debatable - we use the same pattern for the table handles so maybe remove it in both cases?

@mwonng
Copy link

mwonng commented Oct 28, 2024

Hi @YousefED @matthewlipski , is this feature for drag or it is for click, it looks like a button more than a dragable handler. and when drag column (right handler) the column width is changed, is this should be keep the same?

@YousefED
Copy link
Collaborator

Hi @YousefED @matthewlipski , is this feature for drag or it is for click, it looks like a button more than a dragable handler. and when drag column (right handler) the column width is changed, is this should be keep the same?

both clicking and dragging works!

@mwonng
Copy link

mwonng commented Oct 28, 2024

I notice that if table column width(cell width) is undefined and drag extension button will cause width change(see video). but if you manually change the column width, it will keep it. fyi
https://github.com/user-attachments/assets/af82a154-fc27-4ae7-bc97-b471ffa3c719

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.

Table: Cannot read properties of null ChildNodes Table doesn't save columns width after refresh
3 participants