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: add new option defaultCellMinWidth for columnResizing() #253

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

YousefED
Copy link
Contributor

@YousefED YousefED commented Oct 25, 2024

This PR separates cellMinWidth into cellMinWidth and defaultCellMinWidth.

With this, we can create new columns that are a bit wider (100px default) and have some "breathing space", while still have automatic (not fixed sizing). These columns can still be resized by the user to a width smaller than the initial "auto" width (defaultCellMinWidth) up to a minimum of cellMinWidth.

(Note: I think this is relevant only when your table is not set to 100% width)

@ocavue
Copy link
Collaborator

ocavue commented Oct 25, 2024

I found that the options cellMinWidth / resizeMinWidth could be confused, as both options are only valid "when it doesn't have an explicit width set".

What do you think of the following options design:

  • cellMinWidth: The minimal width of a cell / column.

    (The hard limitation. You won't bypass this limitation in any cases. Easy to understand. This probably should be a smaller number like 25px);

  • defaultCellMinWidth: The minimal width of a cell / column when it doesn't have an explicit width set.

    (The soft limitation. It's especially useful for new created column, where there isn't any text to "expand" the column but we still don't want the column to be too dense. This is a soft limitation because it will be invalid once there is an explicit width set, i.e. the resizing handle been dragged. This probably should be a larger number like 120px).

@ocavue ocavue changed the title Separate cell widths feat: add new option defaultCellMinWidth for columnResizing() Oct 25, 2024
Copy link
Collaborator

@ocavue ocavue left a comment

Choose a reason for hiding this comment

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

Thanks @YousefED

@ocavue ocavue merged commit 662e857 into ProseMirror:master Oct 25, 2024
2 checks passed
@@ -50,6 +57,7 @@ export type Dragging = { startX: number; startWidth: number };
export function columnResizing({
handleWidth = 5,
cellMinWidth = 25,
defaultCellMinWidth = 100,

Choose a reason for hiding this comment

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

While I agree with the intention behind the PR, I just spent a bunch of time trying to figure out where this 100px min-width was coming from since it was breaking my tests at https://github.com/ueberdosis/tiptap

I think this might actually be considered a breaking change, because now the defaults have changed from being a minimum of 25px to a minimum of 100px. So rendering the same table with the version prior & the new version can render differently

The only path that I see forward here is to change the default of 100 to be 25 or to cellMinWidth to keep the previous behavior but maintain the new capability (and later on we can introduce change in a major later)

@ocavue @YousefED

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.

3 participants