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

Add page header #69

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

Add page header #69

wants to merge 2 commits into from

Conversation

warcup
Copy link

@warcup warcup commented Sep 4, 2017

add header

@Galileuu
Copy link

Galileuu commented Nov 3, 2017

A little question about this pull request. This code is for adding page headers for the word document?

@ysbaddaden
Copy link

This is working nicely, and will display a document header on all pages.
Sadly, it only supports a single text.

refs #24

@jdugan
Copy link
Contributor

jdugan commented Nov 7, 2017

Hi, all. Apologies for being so absurdly out of pocket on this exchange. My family moved overseas this summer and we've been busy navigating a new culture for the past few months. It's been easy to lose track of Caracal.

A few thoughts:

  • In principle this looks fine, but I tend to agree with @ysbaddaden that I'd prefer a solution that allowed for a greater range of commands—tables, paragraphs, lists, etc.
  • In fairness, my own footer implementation is no more sophisticated than this header implementation, so please understand that I'm not casting any aspersions with the previous comment. :)
  • I'm inclined to accept this as a good faith beginning effort on headers, with the idea that we'd try to tackle issue 24 properly in the future.
  • Assuming you all agree with that let's do this short-term and something better long-term idea, someone will need to update the README to describe the new syntax (and, ideally, to state our future intentions via some sort of footnote).

Does that seem reasonable?

@jdugan
Copy link
Contributor

jdugan commented Nov 7, 2017

I'd also like to see test code for the new objects. :)

@ysbaddaden
Copy link

ysbaddaden commented Nov 7, 2017

I pushed a commit to have whatever into the page header. I needed to have a table and an image, and tried a paragraph. I didn't test lists, but it should be working.
https://github.com/ysbaddaden/caracal/commit/f18c4ab20e33305a74ad8c8f60e1dba8f7190a2c

It relies on some hacks, for example using a decorator on document to transparently collect relationships for the header only (it needs a distinct header1.xml.rels file), and copying the page width/margins so table can calculate its width.

It's still missing specs. I'll try to write some a little later. Now with specs for HeaderModel and #header in the follow up commit:
https://github.com/ysbaddaden/caracal/commit/a4c435e8099936fb2a9f40bb18a6aa588e6f1d14

@jdugan jdugan changed the title a little change Add page header Dec 31, 2017
@njcameron
Copy link

hi @jdugan just following up and seeing if this PR is merge-worthy and if not, what more you need?

@jdugan
Copy link
Contributor

jdugan commented Oct 31, 2018

I think the consensus is that this PR is maybe too hastily constructed for inclusion. More of a high-quality, proof-of-concept than a bullet-proof feature.

My thoughts on an ideal solution are in the comments on this issue.

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.

5 participants