Content box #326

New Issue

2017-01-06T12:41:19-08:00

arcanis commented

2017-01-06 12:41:19 -08:00

(Migrated from github.com)

Now that percent unit support has been added to the library, I was wondering if you've considered exposing the content box on top of the element box. What do you think? It currently seems hard to know the exact size of the padding inside the box.

emilsjolander commented

2017-01-10 03:57:47 -08:00

(Migrated from github.com)

I would prefer to only support border-box sizing. Can you give an example of what is hard the achieve with the current model?

arcanis commented

2017-01-10 04:18:14 -08:00

(Migrated from github.com)

Oh, sure, I completely agree with using border-box. My point was that because units can now be percents, some values need to be resolved one way or the other to be added or substracted to others. For example, let's say I want to get the content width of an element like this:

<div style="width: 500px; padding-left: 5%; padding-right: 10%;" />

I currently need to apply the resolve logic myself:

let contentWidth = myDiv.getComputedWidth();
contentWidth -= myDiv.getPadding(EDGE_LEFT).value / myDiv.getParent().getComputedWidth() * 100;
contentWidth -= myDiv.getPadding(EDGE_RIGHT).value / myDiv.getParent().getComputedWidth() * 100;

Which means that I have to embed some layout logic inside my code. Thankfully, most of those relative properties (margin & padding) all depends on the parent width, otherwise it would have required extra steps to detect whether we should use the parent width or height.

To give you a better idea of what I mean, here is a function I've been using when experimenting with Yoga. I directly added it into the Javascript port, but it could also be implemented inside the library itself (note that it also automatically resolve EDGE_HORIZONTAL into EDGE_LEFT + EDGE_RIGHT, and EDGE_VERTICAL into EDGE_TOP + EDGE_BOTTOM):

for (let fnName of [ `Margin`, `Border`, `Padding` ]) {

	let method = lib.Node.prototype[`get${fnName}`];

	if (method == null)
		throw new Error(`Assertion failed; get${fnName} seems meesing`);

	patch(lib.Node.prototype, `getComputed${fnName}`, function getComputed(_, edge) {

		if (edge === constants.EDGE_HORIZONTAL)
			return getComputed.call(this, _, constants.EDGE_LEFT) + getComputed.call(this, _, constants.EDGE_RIGHT);

		if (edge === constants.EDGE_VERTICAL)
			return getComputed.call(this, _, constants.EDGE_TOP) + getComputed.call(this, _, constants.EDGE_BOTTOM);

		let value = method.call(this, edge);

		if (typeof value === `number`)
			return value;

		switch (value.unit) {

			case constants.UNIT_UNDEFINED: {
				return constants.UNDEFINED;
			} break;

			case constants.UNIT_PIXEL: {
				return value.value;
			} break;

			case constants.UNIT_PERCENT: {
				let relativeSize = this.getParent() ? this.getParent().getWidth() : 0;
				return value.value * relativeSize / 100;
			} break;

		}

	});

}

Oh, sure, I completely agree with using border-box. My point was that because units can now be percents, some values need to be resolved one way or the other to be added or substracted to others. For example, let's say I want to get the content width of an element like this: ```html <div style="width: 500px; padding-left: 5%; padding-right: 10%;" /> ``` I currently need to apply the resolve logic myself: ```js let contentWidth = myDiv.getComputedWidth(); contentWidth -= myDiv.getPadding(EDGE_LEFT).value / myDiv.getParent().getComputedWidth() * 100; contentWidth -= myDiv.getPadding(EDGE_RIGHT).value / myDiv.getParent().getComputedWidth() * 100; ``` Which means that I have to embed some layout logic inside my code. Thankfully, most of those relative properties (margin & padding) all depends on the parent *width*, otherwise it would have required extra steps to detect whether we should use the parent width or height. To give you a better idea of what I mean, here is a function I've been using when experimenting with Yoga. I directly added it into the Javascript port, but it could also be implemented inside the library itself (note that it also automatically resolve EDGE_HORIZONTAL into EDGE_LEFT + EDGE_RIGHT, and EDGE_VERTICAL into EDGE_TOP + EDGE_BOTTOM): ```javascript for (let fnName of [ `Margin`, `Border`, `Padding` ]) { let method = lib.Node.prototype[`get${fnName}`]; if (method == null) throw new Error(`Assertion failed; get${fnName} seems meesing`); patch(lib.Node.prototype, `getComputed${fnName}`, function getComputed(_, edge) { if (edge === constants.EDGE_HORIZONTAL) return getComputed.call(this, _, constants.EDGE_LEFT) + getComputed.call(this, _, constants.EDGE_RIGHT); if (edge === constants.EDGE_VERTICAL) return getComputed.call(this, _, constants.EDGE_TOP) + getComputed.call(this, _, constants.EDGE_BOTTOM); let value = method.call(this, edge); if (typeof value === `number`) return value; switch (value.unit) { case constants.UNIT_UNDEFINED: { return constants.UNDEFINED; } break; case constants.UNIT_PIXEL: { return value.value; } break; case constants.UNIT_PERCENT: { let relativeSize = this.getParent() ? this.getParent().getWidth() : 0; return value.value * relativeSize / 100; } break; } }); } ```

emilsjolander commented

2017-01-10 05:46:05 -08:00

(Migrated from github.com)

Ah ok this has recently been added to the C version. Seems like the javascript wrapper is missing just 👍 https://github.com/facebook/yoga/blob/master/yoga/Yoga.h#L200 gets the computed layout padding after calculation.

emilsjolander commented

2017-01-10 06:07:54 -08:00

(Migrated from github.com)

@arcanis does this look like what you need?

arcanis commented

2017-01-10 10:09:23 -08:00

(Migrated from github.com)

Sounds like it! 👍 How would you feel about adding border & margin counterparts, and the EDGE_HORIZONTAL & EDGE_VERTICAL support? Can I open a PR with these changes?

emilsjolander commented

2017-01-10 10:11:34 -08:00

(Migrated from github.com)

I would be fine with border and margin support. I'm not sure about EDGE_HORIZONTAL & EDGE_VERTICAL support as i'm not certain what that would mean? If you get the horizontal padding of a node with 5 in left padding but 10 in right padding then what should it return?

arcanis commented

2017-01-10 10:18:08 -08:00

(Migrated from github.com)

It would return 15, the sum of left + right. So computing the content width would work like this:

let contentWidth = node.getComputedWidth();
contentWidth -= node.getComputedBorder(EDGE_HORIZONTAL);
contentWidth -= node.getComputedPadding(EDGE_HORIZONTAL);

Instead of:

let contentWidth = node.getComputedWidth();
contentWidth -= node.getComputedBorder(EDGE_LEFT);
contentWidth -= node.getComputedBorder(EDGE_RIGHT);
contentWidth -= node.getComputedPadding(EDGE_LEFT);
contentWidth -= node.getComputedPadding(EDGE_RIGHT);

It would return 15, the sum of left + right. So computing the content width would work like this: ```js let contentWidth = node.getComputedWidth(); contentWidth -= node.getComputedBorder(EDGE_HORIZONTAL); contentWidth -= node.getComputedPadding(EDGE_HORIZONTAL); ``` Instead of: ```js let contentWidth = node.getComputedWidth(); contentWidth -= node.getComputedBorder(EDGE_LEFT); contentWidth -= node.getComputedBorder(EDGE_RIGHT); contentWidth -= node.getComputedPadding(EDGE_LEFT); contentWidth -= node.getComputedPadding(EDGE_RIGHT); ```

emilsjolander commented

2017-01-10 10:20:47 -08:00

(Migrated from github.com)

I don't think we should add that as it works differently than setting horizontal padding (setMargin(EDGE_HORIZONTAL, 10) sets 10 on both sides) and it is easy enough to get left and right and add them together 👍 .

arcanis commented

2017-01-10 10:37:45 -08:00

(Migrated from github.com)

Ok :) How do you feel about implementing YGNodeGetBorder if we don't support additional edge values? Because the borders are unitless, it would just be an alias to YGStyleGetBorder. I think that this alias should be implemented, because even if those functions have the same return value they won't have the same semantic, but it might be controversial. What do you think?

Ok :) How do you feel about implementing `YGNodeGetBorder` if we don't support additional edge values? Because the borders are unitless, it would just be an alias to `YGStyleGetBorder`. I think that this alias should be implemented, because even if those functions have the same return value they won't have the same semantic, but it might be controversial. What do you think?

emilsjolander commented

2017-01-10 10:54:20 -08:00

(Migrated from github.com)

@arcanis I agree. Please split it up in two PRs though.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#326