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

Fix inconsistent margins #1248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikemeerschaert
Copy link
Contributor

Note: this PR includes breaking changes, so should probably be released for v4

tldr;

  • Changed all margins to accept inches instead of points for text
  • Changed processing order of text margin array from LRBT to TRBL so it's consistent with table margins
  • Updated demos so they supply margins in inches

Longer description

I noticed a few inconsistencies between how table cells handle margins and how text items handle margins.
Docs for text:
image

Docs for tables:
image

for tables, the docs say you can use an array in TRBL format, and there is an undocumented feature where you can supply margins in inches, which is very helpful since the PowerPoint UI also shows margins in inches, so if you are copying a design made in PowerPoint into code you can just use the inch values.

Text items also accept an array for margins, but it's undocumented, and processed LRBT, so even if you find the doc for table cells and deduce that it's supported for text, you're likely to be tripped up by the different order. On top of that, margins have to be in points for text, but for tables margins values < 1 are processed as inches, and >= 1 are processed as points.

Since v4 is being released soon, I recommend switching the docs to specify that margins should be supplied in inches rather than points since it's a lot more helpful, and add the margin array doc to text. This pr does that switch. I also noticed there is some code for allowing table cells to accept either points or inches, but the way the logic is structured it wouldn't work for margins over 1". That probably won't come up for table cells, but it might for text, so I don't think the logic is very sound to use for text.

*changed all margins to accept inches instead of points for text
*Changed processing order of margin array to be TRBL so it's consistent with table margins
*Updated demos so they supply margins in inches
@gitbrent gitbrent added this to the 4.0.0 milestone May 20, 2023
@gitbrent gitbrent self-assigned this May 20, 2023
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.

2 participants