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

fix: use toggleHeader from prosemirror-tables #2412

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

jpobley
Copy link
Contributor

@jpobley jpobley commented Jan 20, 2022

This PR will update the tables extension to use the toggleHeader command instead of the deprecated toggleHeaderColumn and toggleHeaderRow commands from prosemirror-tables. This is a breaking change because the commands will now only change the first column or row, respectively, instead of the row(s) or column(s) in the current selection. See the attached videos for examples.

The exception to the changes is toggleHeaderCell. I'm not sure what we'd like to be done with it and am looking for some feedback. Currently, it toggles any cell(s) in the selection to be "header cells". This is the deprecated behavior in prosemirror-tables. If we change it to use toggleHeader, then it will only toggle cells that are within the first row or column and, even then, it will only remove a header cell, it doesn't put one back. This is confusing which is why I haven't included that change here.

Old Behavior

OldBehavior.mp4

New Behavior

NewBehavior.mp4

Switches the table commands to use `toggleHeader` command instead of the deprecated `toggleHeaderColumn`, `toggleHeaderRow`, and `toggleHeaderCell` commands from `prosemirror-tables`.
@netlify
Copy link

netlify bot commented Jan 20, 2022

✔️ Deploy Preview for tiptap-embed ready!

🔨 Explore the source changes: e8a90cc

🔍 Inspect the deploy log: https://app.netlify.com/sites/tiptap-embed/deploys/61e9de000144f90007eee8a0

😎 Browse the preview: https://deploy-preview-2412--tiptap-embed.netlify.app

@hanspagel
Copy link
Contributor

Thanks for the PR! ☀️

@philippkuehn
Copy link
Contributor

Great, this is also a fix for #548.

@codemzy
Copy link
Contributor

codemzy commented Jan 26, 2022

@jpobley nice work!! I think it makes sense that toggleHeaderRow applies to the first row. I'd like to mention that it can be useful to have th in other places, I currently use tiptap for creating forms etc and toggleHeaderCell to create tables like this...

Screenshot 2022-01-26 at 11 23 47

So if that functionality can be maintained (since you asked for feedback!) it would certainly be useful for some use cases.

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.

None yet

4 participants