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

OpenDocument writer: Table width support #6792

Merged
merged 1 commit into from
Nov 21, 2020

Conversation

pyssling
Copy link
Contributor

Use the same key-value in Attr for the table as the HTML writer
to implement table width support.

Copy link
Collaborator

@tarleb tarleb left a comment

Choose a reason for hiding this comment

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

Thanks! I left some minor code comments. LGTM otherwise!

src/Text/Pandoc/Writers/OpenDocument.hs Outdated Show resolved Hide resolved
src/Text/Pandoc/Writers/OpenDocument.hs Outdated Show resolved Hide resolved
@pyssling pyssling force-pushed the opendocument-table-width-support branch from 5fbdd32 to 6132282 Compare November 3, 2020 22:04
@tarleb
Copy link
Collaborator

tarleb commented Nov 3, 2020

Just to keep you in the loop: a new release is in preparation right now. That means merging may be delayed until after the release is out.

@jgm
Copy link
Owner

jgm commented Nov 4, 2020

Can you explain more fully what you mean by "table width support"?

@pyssling
Copy link
Contributor Author

pyssling commented Nov 4, 2020

Yes sorry - this is the width of the table as percent of the page-width. It's correctly transferred into the HTML writer.

@jgm
Copy link
Owner

jgm commented Nov 4, 2020

This should be determined by the relative widths of the columns, as specified in the table AST, right?

@pyssling
Copy link
Contributor Author

pyssling commented Nov 4, 2020

Column widths come in as a percentage of the table width as far as I can tell. I.e. you still need the full width of the table.

The AST types define the following:

-- | The width of a table column, as a fraction of the total table
-- width.

I.e. they seem to be relative to the total table width - not absolute.

@jgm
Copy link
Owner

jgm commented Nov 4, 2020

No, actually, the proper interpretation of the relative column widths in the table AST is as percentages of text width (not table width).

@pyssling
Copy link
Contributor Author

pyssling commented Nov 4, 2020

Ok, I can change it to do that and send a pull-request to fix the AST documentation then.

@pyssling
Copy link
Contributor Author

pyssling commented Nov 4, 2020

@jgm here is a pull-request to correct the AST documentation: jgm/pandoc-types#85

If you could review and it's correct then I can refactor this pull-request and the one for the DocBook reader.

@pyssling pyssling force-pushed the opendocument-table-width-support branch from 6132282 to 33ba870 Compare November 5, 2020 22:13
@pyssling
Copy link
Contributor Author

pyssling commented Nov 5, 2020

@jgm I've refactored this now to make use of the column widths as a percentage. Much more satisfying than looking at an undocumented key-value I have to say.

@jgm
Copy link
Owner

jgm commented Nov 7, 2020

I'm just wondering how this interacts with the relative column widths. Do we support column widths yet in opendocument? If we do, how do these interact with the table width?

@pyssling
Copy link
Contributor Author

pyssling commented Nov 7, 2020

Yes - column widths are supported and work from what I've seen - they are expressed in proportion to each other - i.e. they are without unit.
It works perfectly from what I can see - it really just scales the table in relation to text-width - doesn't affect the columns at all - they are still proportional as before.

@jgm
Copy link
Owner

jgm commented Nov 17, 2020

How about a test or two? Easiest way is to add a command test, which could be named test/command/6792.md.

@pyssling
Copy link
Contributor Author

I'll see what I can do - this requires a full test - command tests do not work for styles as they can only manage opendocument output - not odt output.

@pyssling
Copy link
Contributor Author

I'm afraid this is beyond me - there is no framework for testing the ODT writer - just the ODT reader. I'm afraid I wouldn't know how (or have the time) to develop that sort of framework. I'm afraid these styles will remain untested for the time being.

@jgm
Copy link
Owner

jgm commented Nov 19, 2020

Since this modifies the opendocument writer, couldn't we just have a command test with pandoc -t opendocument?

@pyssling pyssling force-pushed the opendocument-table-width-support branch from 33ba870 to 9613b90 Compare November 20, 2020 08:18
@pyssling
Copy link
Contributor Author

Ok, thank you for pushing me on this. I figured out what was going on - the opendocument writer doesn't emit styles unless a template is used - so I got that working. Now there's a solid unit test. Thank you.

@pyssling pyssling force-pushed the opendocument-table-width-support branch from 9613b90 to e7f091a Compare November 20, 2020 08:29
@jgm
Copy link
Owner

jgm commented Nov 21, 2020

Great. I don't think we need the special template. Just use the default one:
pandoc -f native -t opendocument -s.

Support for table width as a percentage of text width by summing
width of columns and verifying that the sum is > 0 and <= 1.
@pyssling pyssling force-pushed the opendocument-table-width-support branch from e7f091a to 9813c0e Compare November 21, 2020 09:03
@pyssling
Copy link
Contributor Author

done, sorry about that - didn't realize the template directory was accessible.

@jgm jgm merged commit ae52918 into jgm:master Nov 21, 2020
@jgm
Copy link
Owner

jgm commented Nov 21, 2020

great, thanks!

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.

3 participants