Add support for dynamic sizing #1576
Conversation
hollandjake
force-pushed
the
dynamic-sizing
branch
from
632a544 to
f19a3e8
Compare
hollandjake
force-pushed
the
dynamic-sizing
branch
from
2ae9b06 to
0f0fb9e
Compare
|
LGTM |
|
@blikblum let me know if you want this added to all the mixins too? |
| if (size > 0) return size; | ||
| return defaultValue; | ||
| } | ||
| if (typeof size === 'boolean') return Number(size); |
There was a problem hiding this comment.
What situation a boolean value makes sense?
There was a problem hiding this comment.
Its mainly for a falsy value of saying margin: false to indicate that you don't want a margin to render at all
There was a problem hiding this comment.
true would return 1 -> does not make sense
I am not sure how useful would be.
margin: 0 seems enough to not have a margin
There was a problem hiding this comment.
if we are totally happy with not having margin: false i'm happy removing it, I only added it so devs could make it explicitly clear they don't want a margin, rather than confusingly saying they want a margin of width 0
It also introduce new ways to define margin e.g. with an array of number |
Good point, it mainly aims to unify all the units across the package for consistent definitions |
hollandjake
force-pushed
the
dynamic-sizing
branch
from
0f0fb9e to
33541a2
Compare
There was a problem hiding this comment.
I keep my two considerations.
The first is difference how margin is defined in an array different from pdfmake. In favor of your implementation is that is aligned with css spec. I'd like to know what other devs think
The second is a NIT the boolean handling in size to point. While it does not make sense, it does not hurt
| * Side definitions | ||
| * - To define all sides, use a single value | ||
| * - To define up-down left-right, use a `[Y, X]` array | ||
| * - To define each side, use `[top, right, bottom, left]` array |
There was a problem hiding this comment.
This is different how pdfmake defines: https://pdfmake.github.io/docs/0.1/document-definition-object/margins/
It can cause confusion
To be fair, the way you defined is compatible with the CSS order: /* top | right | bottom | left */ -> https://developer.mozilla.org/en-US/docs/Web/CSS/margin
There was a problem hiding this comment.
My goal was always to make it more like CSS than pdfmake. Im not quite sure the reason why pdfmake didnt do the same
There was a problem hiding this comment.
I don't know why the parameters are defined in this order in pdfmake, but it would be better to use the CSS order in pdfkit.
| if (size > 0) return size; | ||
| return defaultValue; | ||
| } | ||
| if (typeof size === 'boolean') return Number(size); |
There was a problem hiding this comment.
true would return 1 -> does not make sense
I am not sure how useful would be.
margin: 0 seems enough to not have a margin
|
@hollandjake the tests are failing |
@blikblum will investigate now, most likely a regression from merging in main |
hollandjake
force-pushed
the
dynamic-sizing
branch
from
33541a2 to
03e2f6e
Compare
- Enable defining sizes using any units (defaulting to Points) - This also allows us to define sizes based on the current font context i.e. em's - The new public `sizeToPoint` method allows users to also interact with these sizes to generate the correct point sizes
hollandjake
force-pushed
the
dynamic-sizing
branch
from
03e2f6e to
ea77557
Compare
lib/utils.js
Outdated
| !( | ||
| "top" in sides || | ||
| "right" in sides || | ||
| "bottom" in sides || | ||
| "left" in sides | ||
| ) |
There was a problem hiding this comment.
I believe {top: 1, right: 1} should get invalid value: {top: 1, right: 1, left: {top: 1, right: 1}, bottom: {top: 1, right: 1}}
Or do i miss something?
There was a problem hiding this comment.
I would remove this check at all assuming is undefined is treated as 0. If i do {top: 1, right: 1} i expect left and bottom be treated as 0
There was a problem hiding this comment.
the idea is to allow partials, so if anyone provides at least one of the keys it will be used and the others set to undefined, however if none of the keys match the object itself will be treated as the value for all keys.
e.g. {top: 1} would produce {top: 1, right: undefined, bottom: undefined, left: undefined}
but {hello: "world"} would produce {top: {hello: "world"}, right: {hello: "world"}, bottom: {hello: "world"}, left: {hello: "world"}}
There was a problem hiding this comment.
other option we could take is to just say if its an object and you didnt provide the keys then it'll be undefined
There was a problem hiding this comment.
thinking about it now, it really doesnt make any sense to provide this, its the users fault
There was a problem hiding this comment.
@blikblum please see new changes which removes this logic
What kind of change does this PR introduce?
sizeToPointmethod allows users to also interact with these sizes to generate the correct point sizesChecklist: