Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

issue calculating the available height for a table using Auto paging #64

Closed
tb23911 opened this issue Mar 21, 2017 · 4 comments
Closed
Assignees
Milestone

Comments

@tb23911
Copy link

tb23911 commented Mar 21, 2017

When creating a table using

		slide.addTable( arrRows, 
			{ 
				slideMargin:  [ 1.2, 0.0, 1.18, 0.0 ], 
				x:0.5, 
				y:1.2, 
				colW:[5.0,0.55], 
				font_size:9, 
				margin:0, 
				border:{pt:'0.75', color:'E00000'}, 
				debug:true 
			} 
		);

Issue 1: If you do not specify the cell margins a default seems to be added (not sure where this is added) of [3.5, 7, 3.5, 7] in POINT ([0.13, 0.25, 0.13, 0.25] in cm). When calculating the available height in this scenario the default margins are not added to the lineHeight.

Issue 2: If you have specified the cell margins in the arrRows, this is added to the lineHeight variable but in Point rather than EMU, there by allowing the table to extend past the bottom slide margin and in some cases the bottom of the slide.

The check is in the function getSlidesForTableRows within this loop:

			// C: Parse and store each cell's text into line array (*MAGIC HAPPENS HERE*)
			row.forEach(function(cell,iCell){
			
				...

	->			if ( Array.isArray(cell.opts.margin) && cell.opts.margin[0] ) lineHeight += cell.opts.margin[0] / intMaxLineCnt;
	->			if ( Array.isArray(cell.opts.margin) && cell.opts.margin[2] ) lineHeight += cell.opts.margin[2] / intMaxLineCnt;

				...

			});
@tb23911
Copy link
Author

tb23911 commented Mar 22, 2017

I notice that you have the function calcEmuCellHeightForStr and the comments for this function suggests that it is used by parseTextToLines, but looking at this function and the rest of the library it doesn't seem to be used at all.

One thing the function doesn't see to cover is where the cell margins are not defined and the default is applied.

@gitbrent
Copy link
Owner

gitbrent commented Apr 3, 2017

Hi @tb23911 ,

Thanks for opening an issue.

I've updated the code to define the default margins at the very top now:
var DEF_CELL_MARGIN_PT = [3, 3, 3, 3];

You're right about the conversion of margin points to EMU not happening. It's been fixed now as well.

calcEmuCellHeightForStr() was a legacy function that i'd neglected to remove. Thanks for the good eyes.

@tb23911
Copy link
Author

tb23911 commented Apr 3, 2017

Hi @gitbrent ,

Just tested it and it looks good.

One thing I noticed is where you have a column that is quite narrow and the content of that cell is 1 continuous word and PowerPoint does a hard wrap, this messes up the row height and therefore the calculations for when to break onto a new page.

Another thing; when calculating the lineHeight you may need to consider including the border widths as this can skew the calculation.

I noticed that you have used a constant in the function parseTextToLines when calculating the lineHeight, was that constant based on an average? The reason I ask is that for non-fixed width fonts this is likely to change, and even worse change on a per character basis. For example I am using Arial which invariably uses less characters per line than what is calculated using the constant of 2.2.

Last point; I noticed whilst stepping through the code that the block where you try calculate the opts.w in the function getSlidesForTableRows, the if block checks that opt.w is not defined and opts.colW is, then you loop through the columns and add the widths to opts.w, but because opts.w has not been defined, the += operand does not work. the simple fix will be to assign the value of 0 before the loop, like:

		// Calc opts.w if we can
		if ( !opts.w && opts.colW ) {
	------->	opts.w = 0;
			if ( Array.isArray(opts.colW) ) opts.colW.forEach(function(val,idx){ opts.w += val });
			else { opts.w = opts.colW * numCols }
		}

Great work though... I have some enhancement requests too which I will create separately.

@ZouhaierSebri ZouhaierSebri mentioned this issue Apr 6, 2017
Closed
gitbrent pushed a commit that referenced this issue Apr 8, 2017
@gitbrent
Copy link
Owner

gitbrent commented Apr 8, 2017

Hi @tb23911 ,

  • The fixed line height modifier is loosely based on the Golden Ratio for fonts.
  • Use of the lineWeight option allows users to customize the modifier as needed to fit any type of font usage
  • Small columns and text wrapping cant be detected, so i don't know how to solve this other than to let users fit it to their needs.

Lastly, a w value is now always created in the addTable() function do downstream processes can utilize it safely.

Thanks for the feedback.

@gitbrent gitbrent closed this as completed Apr 8, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants