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

Basic table support (v2) #77

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Basic table support (v2) #77

merged 4 commits into from
Nov 7, 2022

Conversation

clement-escolano
Copy link
Contributor

Hello

Thank you for this library. I am also interested in table support and saw there was already an open pull request.
I took the first commit unchanged and changed the tests as you asked and also took into account some feedback.

What do you think?

Thank you

table = self.tables.pop()[0]
section = self.doc.sections[0]
page_width = section.page_width - section.left_margin - section.right_margin
page_width = int(page_width * (0.5 ** len(self.tables)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain this line? I don't follow why the page width is multiplied by 0.5 ^ len(self.tables). Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't. I did not wrote this line, this is from a previous pull request (#55), I just took the existing code and worked on the tests which was the main feedback.
If I had to guess: this is an estimation of the width of the table which is smaller the more nested is the table.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately the original reviewer is no longer with our organization. I can't approve this PR until I fully understand what this line does. If you like you can fork the repo and merge it in on your fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I understand. This line is originally to handle nested table which I think is fairly rare. If you agree, I will just remove the "nested" feature so that we can start with this simple case and then we can add more edge cases in the a future pull request?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's a good idea

@clement-escolano
Copy link
Contributor Author

@kbattersby Do we need something else or we can merge this? 🙂

@kbattersby
Copy link
Contributor

@kbattersby Do we need something else or we can merge this? slightly_smiling_face

It's been approved and all checks passed, so you can merge it.

@clement-escolano
Copy link
Contributor Author

Unfornutanetely I don't see the button 🤷
image

@kbattersby kbattersby merged commit cefccbb into erezlife:master Nov 7, 2022
@kbattersby
Copy link
Contributor

@kbattersby Do we need something else or we can merge this? slightly_smiling_face

It's been approved and all checks passed, so you can merge it.

@clement-escolano OK I did it

@clement-escolano
Copy link
Contributor Author

Thank you

@clement-escolano
Copy link
Contributor Author

@kbattersby Is it possible to publish a new version please? Right now, I must install the package from GitHub instead of pypi to use this feature. Thank you

@clement-escolano
Copy link
Contributor Author

@kbattersby Sorry to insist, is it possible to publish a new version in the near future? Thank you

@kbattersby
Copy link
Contributor

kbattersby commented Dec 16, 2022

Ok we will do that. Most likely I'll be able to release it within the next week or 2.

@kbattersby kbattersby mentioned this pull request Jan 3, 2023
@clement-escolano
Copy link
Contributor Author

@kbattersby Thank you, I see you upgraded the version in the repository but I don't see it in pypi. There is probably an extra step missing.

@kbattersby
Copy link
Contributor

Yes I still need to release it. I've had some other tasks on my plate but I plan to get back to this quite soon.

@kbattersby
Copy link
Contributor

@clement-escolano It should be released now. Let me know if you have any problems. Thanks!

@clement-escolano
Copy link
Contributor Author

@kbattersby Thank you 🙏

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