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

Bugfix for flowing text always adding new page #1202

Closed
wants to merge 6 commits into from
Closed

Bugfix for flowing text always adding new page #1202

wants to merge 6 commits into from

Conversation

rexxmagnus
Copy link

Allow text added on a page to flow onto the next page if it already exists, rather than always adding a new page below.

Bugfix
#1201

What kind of change does this PR introduce?

Text added to any page that is followed by another can flow onto the next existing page. This fixes a bug that would always add a new page regardless of whether the page is the last one or not.

Checklist:

  • Unit Tests N/A
  • Documentation
  • Update CHANGELOG.md
  • Ready to be merged

Manual tests have been run in the case of using buffered and unbuffered page output.

Allow text added on a page to flow onto the next page if it already exists, rather than always adding a new page below.
Update to include new bugfix for text flowing onto existing pages.
Added note about text flow when switching to previous pages.
Removed redundant double-negation.
Copy link
Author

@rexxmagnus rexxmagnus left a comment

Choose a reason for hiding this comment

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

Removed redundant !! from code to pass build tests.

@rexxmagnus rexxmagnus changed the title Patch 1 Bugfix for flowing text always adding new page Dec 23, 2020
@rexxmagnus
Copy link
Author

This will require re-testing with the amend of this.document.addPage() to this.document.continueOnNewPage();

@blikblum
Copy link
Member

Sorry for the delay.

I'm trying to cut a new release.

Could you add a sample example and a unit test?

@liborm85 @devongovett can you review if this behavior is desired?

@rexxmagnus
Copy link
Author

Sorry for the delay.

I'm trying to cut a new release.

Could you add a sample example and a unit test?

@liborm85 @devongovett can you review if this behavior is desired?

Just to clarify, the original issue this is meant to correct was from a use-case where I was producing a two-column layout which was intended to flow onto the next page. See the issue #1201 for a minimal code sample.

@blikblum
Copy link
Member

Thanks.

I will wait for other dev opinions on the behavior. BTW: the two column layout example seems not the best one since AFAIK, a complete column implementation is a lot more involved: when a text in the left column overflows should be to continue on right column in the same page. And when the right column overflows should continue on the left column in next page.

A better API would allow to customize what to do when text overflows, add page or not and from what position restarts

Anyway having unit tests and small examples in demo folder improves the likehood of being merged. See recent attachment and form PRs.

@rexxmagnus
Copy link
Author

rexxmagnus commented Mar 28, 2021

I agree, having an api to control flow would be better. My case is for columns that span pages vertically, rather than the content continuing from left to right in the next column, hence not using the column structure. I don't think my fix should interfere with that default behaviour, although more tests are needed.

@Simolation
Copy link
Contributor

Simolation commented Mar 31, 2021

For me, this is also a really important behavior as I am placing form fields (with a text description below that could wrap and trigger the page add) and they should be displayed next to each other. When the page is over they should continue on the next page in the same column. So for me, I don't need the text wrapping behavior to the right you mentioned.

In my opinion, adding a new page at the end when the current page is not the last one is never the desired behavior and is just unexpected. At least I can't imagine a valid use case.

Can I somehow help to speed this merge up?

@blikblum
Copy link
Member

Can I somehow help to speed this merge up?

Yes, add a unit test. See current ones to know how can be done

@Simolation
Copy link
Contributor

Simolation commented May 29, 2021

I just added visual tests and slightly improved the code #1256

@blikblum
Copy link
Member

Closed in favor of #1256

@blikblum blikblum closed this May 29, 2021
@rexxmagnus rexxmagnus deleted the patch-1 branch October 13, 2021 14:07
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.

None yet

3 participants