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

[lexical-table] Support first column freeze #7134

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

Conversation

ivailop7
Copy link
Collaborator

@ivailop7 ivailop7 commented Feb 4, 2025

Part one of freeze support in tables.
Have done the easier part, freezing a column

Screen.Recording.2025-02-09.at.17.28.34.mov

Copy link

vercel bot commented Feb 4, 2025

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

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 9, 2025 5:33pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 9, 2025 5:33pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 4, 2025
Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

Seems to work well! I think a number is a bit more future-proof than a boolean (the accessor could be a boolean for now, mostly just thinking about the JSON). I did notice a typo but otherwise this looks good. I only tested in Chrome though, Safari and Firefox might have CSS quirks with this sort of thing?

@@ -48,6 +48,7 @@ export type SerializedTableNode = Spread<
{
colWidths?: readonly number[];
rowStriping?: boolean;
frozenFirstColumn?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For what it's worth, the Google Sheets API uses a frozenRowCount and frozenColumnCount. Freezing the first n rows or columns is probably not much harder than freezing one? Even if we only support 1 right now we could still future-proof the JSON by using a number.

https://developers.google.com/sheets/api/reference/rest/v4/spreadsheets/sheets

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a fantastic suggestion. I'll do it this way.

packages/lexical-table/src/LexicalTableNode.ts Outdated Show resolved Hide resolved
@ivailop7
Copy link
Collaborator Author

ivailop7 commented Feb 9, 2025

@etrepum this one is good to go

Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

This looks like it's missing the exportJSON for frozenColumns (and it doesn't have any tests that exercise serialization). Could be fixed as a follow-up, can easily repro in the editor by copying and pasting a table that has a frozen column

@ivailop7
Copy link
Collaborator Author

ivailop7 commented Feb 9, 2025

Oh, my oversight. I'll add the export JSON and add an extra test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants