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(table): Add table footer support #14276

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

star-szr
Copy link

@star-szr star-szr commented May 29, 2023

Suggested merge commit message (convention)

Fix(table, html-support, source-editing, style): Add support for table footers (<tfoot>). Closes #12952.

MINOR BREAKING CHANGE (table): Conversion helper downcastTableElement() now requires TableUtils as the last argument.

MINOR BREAKING CHANGE (table): tableHeadingsRefreshHandler() now requires TableUtils as the last argument.


Additional information

Example <tfoot> markup: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tfoot

Maybe this is more of a feature than a fix, but at the same time this is just standard HTML support.

  • Still TODO:
    • Add/update manual tests
    • Search through documentation for potential updates
    • Other? It's my first contribution so please let me know if I'm missing anything!

I have intentionally not tried to build any UI around managing table footers at this point, and have not added a SetFooterRowCommand. In my opinion, this proposed set of changes can go in (after review of course) and more UI functionality can be added later.

In addition, I have not tried to add configurable default footer rows to match the "default table headers" feature. The scope already seemed big enough without adding that, and I think it can be added later (I'm not sure there is much use case anyway).


Credits

This work was sponsored by @ongov.

@arkflpc arkflpc requested a review from niegowski May 30, 2023 08:37
@star-szr
Copy link
Author

Working on resolving the conflicts.

@star-szr
Copy link
Author

star-szr commented May 30, 2023

I found one test case in packages/ckeditor5-table/tests/tableclipboard-paste.js that needs to be updated, working on that.

Update: Done.

@Witoso
Copy link
Member

Witoso commented Jun 1, 2023

We will try to take a look at this soon!

@star-szr
Copy link
Author

star-szr commented Jun 13, 2023

Just want to note that the current code assumes that all children of tfoot > tr should be th, which is probably a fair assumption for cells inside thead > tr, but less so for cells inside the footer.

I will be taking a look at this soon and likely updating so that only cells in a footer row that are also in a header column are output as th, others will be td.

@star-szr
Copy link
Author

star-szr commented Jun 15, 2023

Updated to make table footer cells downcast to td unless they are in a header column, and made some other touch-ups that I noticed.

Edit: I'm still reviewing the whole PR to see if there are other pieces that need to be updated to reflect the th to td change. I will post a comment once I think it's ready for review again.

@star-szr star-szr force-pushed the i/12952 branch 2 times, most recently from 6e52f55 to 0d165a7 Compare June 20, 2023 21:01
@star-szr
Copy link
Author

I've reviewed everything again, and updated the logic inside tableHeadingsRefreshHandler() and added test coverage for that. I also rebased on top of master.

This is ready again for review and feedback.

@star-szr
Copy link
Author

star-szr commented Sep 5, 2023

@Witoso please let me know if there's anything else I can do at this point.

@Witoso
Copy link
Member

Witoso commented Sep 8, 2023

Hey, sorry for keeping this open for so long. I need to carve out some time in the team to take a look at this, and we are in the middle of some complex topics. CC @arkflpc to discuss the best time.

@tonprince
Copy link

Any updates about support of tfoot in ckeditor?

@artemdotsenko
Copy link

Any updates about support of tfoot in ckeditor?

Same question 👀

@star-szr
Copy link
Author

To folks who are commenting checking on this, I appreciate you, and also I highly recommend that you click the thumbs up reaction in the original comment here.

@Witoso
Copy link
Member

Witoso commented Dec 4, 2023

Sorry for keeping this open for so long 🙏 . We are battling some end-of-the-year priorities. I hope we will be able to take this to our sprint really soon.

@star-szr

This comment was marked as outdated.

@Megafry
Copy link

Megafry commented Sep 10, 2024

Any updates about when we can expect this to be merged in to master?

@Witoso
Copy link
Member

Witoso commented Sep 10, 2024

We have a plan to work and merge together a few larger tables changes, but we don't have yet clear timeline yet.

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.

<tfoot> element is lost during the conversion
5 participants