Add table spanning support #420
Conversation
dobromir-hristov
requested review from
mportiz08,
QuietMisdreavus,
ethan-kusters,
marinaaisa and
hqhhuang
There was a problem hiding this comment.
The JS code for creating the markup looks good, and I like that it's pretty minimal in terms of what actually changed. The empty cells for older versions of DocC without extended data look how I'd expect to as well—nice job!
I do have some concerns about the change in the visual style that is now happening only for tables that have cells that span. It wasn't obvious why there needs to be a difference between normal tables without any. It seems like the width and border of the table is changing, but it isn't obvious why.
|
@swift-ci test |
|
@marinaaisa no, I have not updated the example since @QuietMisdreavus changed the spec |
|
@swift-ci test |
|
@marinaaisa test it again :) |
|
@marinaaisa @dobromir-hristov It should also be noted that the current implementation in Swift-Markdown and Swift-DocC uses |
Tested it, works like a charm. Thanks. We just need to clear up some styling confusion and we are good to go. |
|
Okay, now it looks as expected :) thanks @dobromir-hristov @QuietMisdreavus !
|
| renderTableChildren(node.rows, node.header) | ||
| return createElement(Table, { | ||
| props: { | ||
| spanned: !!node.extendedData, |
There was a problem hiding this comment.
Is this still needed since the class isn't being used anymore?
There was a problem hiding this comment.
Probably not, just thought it might help future devs, if they want to style these tables in some way with custom styling 🤔
|
@swift-ci test |
Bug/issue #, if applicable: 98017748
Summary
Adds support for Table col and row spanning, based on the initial DocC draft proposal.
Note:
This is still experimental and needs to be synced with actual DocC RenderJSON. For now think of this as a POC.
Dependencies
Testing
Use the provided JSON.
example_tablespan.json.zip
Steps:
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test, and it succeeded