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

Support for plain table + caption in the data pipeline #11240

Merged
merged 25 commits into from
Mar 2, 2022

Conversation

dawidurbanski
Copy link
Contributor

@dawidurbanski dawidurbanski commented Feb 8, 2022

Suggested merge commit message (convention)

Feature (table): Introduced the PlainTableOutput plugin to override default figure>caption markup in the data pipeline. Closes: #10892.

Tests (table): Added tests for the TableCaptionUI plugin.


Please note that as per the original ticket we don't document this feature in any guides.

Let's deliver this as a plugin, with tests. But let's not cover this in the guides, at least for now.

@dawidurbanski dawidurbanski marked this pull request as draft February 8, 2022 18:18
Base automatically changed from ck/10294/reconversion to master February 9, 2022 12:37
@dawidurbanski dawidurbanski changed the title WIP: Naked table + caption support Support for plain table + caption in the data pipeline Feb 14, 2022
@dawidurbanski dawidurbanski marked this pull request as ready for review February 14, 2022 17:02
Copy link
Contributor

@maxbarnas maxbarnas left a comment

Choose a reason for hiding this comment

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

Nice, clean work! 🎉

packages/ckeditor5-table/src/plaintableoutput.js Outdated Show resolved Hide resolved
@Reinmar
Copy link
Member

Reinmar commented Mar 2, 2022

I played with the feature a bit and to me, it looks good in terms of functionality. However, I found some issues that require followups.

  • The manual test should include GHS.
  • After adding GHS I found that <caption>'s attributes are not preserved. Please open a followup ticket for this.
  • After finding ☝️ I realized there are no tests for the integration with GHS. Please open a followup ticket for this.
  • I cleaned the manual test from:
    • a warning logged for missing image toolbar config,
    • unnecessary (in this context) default table properties setting

@Reinmar Reinmar merged commit 9379c5c into master Mar 2, 2022
@Reinmar Reinmar deleted the ck/10892-table-caption branch March 2, 2022 08:58
@Reinmar
Copy link
Member

Reinmar commented Mar 2, 2022

Ooops. I mistakenly rebased the entire branch when fixing one of my commits:facepalm:

Sorry for the future readers of this PR.

*
* See for instance:
*
* * {@link module:table/table~TableConfig#contentToolbar `editor.config.table.contentToolbar`}
Copy link
Member

Choose a reason for hiding this comment

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

It should be config.table.contentToolbar. I will fix this on master.

Copy link
Member

Choose a reason for hiding this comment

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

69db658 done

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.

Provide a solution for how to downcast a table to table>caption
4 participants